IECorePython::ExceptionAlgo : Handle traceback failure

[danieldresser-ie]

This is pretty weird, and I'm not sure if it's actually worth merging, but I need to reference it from an email I'm writing John about a weird Gafffer issue, so it's probably easiest to put it up as a PR.

In certain weird corner cases during shutdown, I am somehow able to produce a situation where importing the "traceback" module fails, which would previously cause a crash. With this PR, we just produce quick partial traceback instead of the properly formatted one, which is enough to help track down the issue.

The second commit is maybe too weird, but it would be nice if we could at least merge the first commit, which makes this scenario a lot clearer. Or maybe once we figure out this weird issue, we will never again encounter a scenario where traceback fails to load, and we can forget about this.

[danieldresser-ie]

This is definitely not good to merge - it's causing crashes elsewhere. Maybe once we can figure out why traceback sometimes fails to load during shutdown we can forget about this.

[johnhaddon]

Couple of links you may or may not have come across in your travels :

https://stackoverflow.com/questions/25649676/where-is-pythons-shutdown-procedure-documented/25649713
https://docs.python.org/3/whatsnew/3.4.html#pep-442-safe-object-finalization
https://github.com/GafferHQ/gaffer/blob/master/python/GafferUI/VectorDataWidget.py#L871

I think these imply that we can't guarantee that traceback won't be None, so we will need a workaround of some sort...

[johnhaddon]

I'm concerned about the crash you mention, and the fact that the new code runs all the time, not just in the exceptional case. If we can isolate the new code to the exceptional case only then I think that would be a big improvement. In that case Python is already in a dire state, so we really can't make anything worse. Will something like this work?

// Your fallback formatting code, as a private utility
std::string poorMansFormatPythonException( object exception, object value, object traceback )
{
}
...
std::string formatPythonException( bool withStacktrace, int *lineNumber )
{
  // As before
   ...
   try
   {
      		object tracebackModule( import( "traceback" ) );
                // As before
                ...
   }
   catch( boost::python::error_already_set &e )
   {
       return poorMansFormatPythonException( exception, value, traceback );
   }
}

I think that's a lot clearer, as well as potentially more robust...

[johnhaddon]

When PyErr_Fetch has returned, we own a reference to ptype, pvalue and ptraceback. In the existing code we use a handle or object to wrap these, and that takes care of dropping the reference automatically. Otherwise the manual reference counting required by the Python API is just too error prone...

[danieldresser-ie]

I had originally tried something like this, but found that after the other stuff had tried to run, it seemed to have somehow invalidated "value", and my simple code wouldn't work any more, which is why I moved it earlier. There doesn't seem to be any good reason for this though, so if you think this is actually the right approach, then we just need to debug that.

[johnhaddon]

I did wonder if that might have been the case, but I couldn't think of a reason why. Worth revisiting briefly I reckon...

[johnhaddon]

After testing this locally I forgot to switch back to master, and discovered that with this branch, I get a crash in Gaffer if I put raise ValueError( 10 ) in an expression. This is fine on master, so it seems there's definitely something amiss...

[johnhaddon]

What are you thinking for this one Daniel? Unless you especially want to take another stab at it, I'm inclined to close it and move on...

[danieldresser-ie]

Still needs fixing, since we need this code to debug certain cases which will likely continue to occur. At Andrew's suggestion, closing PR, and making an issue to keep track of it.