1. Mike Miller
  2. pytave
  3. Pull requests

Pull requests

#15 Merged
Repository
Deleted repository
Branch
default (6fffa6219b2c)
Repository
pytave
Branch
default

Properly extract exception text from Python (fixes issue #24)

Author
  1. Abhinav Tripathi
Reviewers
Description
  • exceptions.{h, cc}: Add a new method to properly extract string for a python exception
  • pycall.cc, pyeval.cc, pyexec.cc: Use the new method to extract string when an exception occurs. Also add a new error message in case we can't extract the string.
  • Issues #24: pyexec: inconsistent error handling resolved

Comments (12)

  1. Abhinav Tripathi author

    This was my first commit with changes in so many files, I hope my commit message follows the octave convention.
    .
    I was just going through the issues and this one seemed interesting (sorry for not posting anything on the issue page), so I did a little digging.
    .
    The actual problem was that boost wasn't able to extract the string from the exception. The execution went into the nested catch block. There PyErr_Print() printed the traceback but no error was raised for octave to notice.
    So, I added the error ("pyexec: Exception caught"); line in each of the 3 pyeval, pycall & pyexec files. We should probably change the error message. But, we definitely need an error statement there to tell octave that error occured.
    .
    Pls review Colin Macdonald Mike Miller

  2. Abhinav Tripathi author

    I just update the commit message a bit.
    .
    Also, I forgot to mention that I tried to extract the string in many other ways like using char* or other data types with extract or using Py* python functions to extract the string. But they didn't work too.
    With these changes, the execution does NOT go into the nested catch block for all the examples mention in #24 and the string is extracted properly for all exceptions.
    I couldn't find a case in which the execution would go into the nested catch block, but still, I think we should have an error message in that too, just to be safe.

  3. Colin Macdonald
    • If I've understood correctly, the error message should be something like "pyexec: Exception encountered while trying to throw exception".

    • But why (error_already_set const &)? If this is some emergency catch-all, why isn't it just catch? (Careful: I don't know anything about try/catch in C++ so possibly I'm wrong about this).

    • You've got pyexec in the error text for all them, but some should be pycall pyeval as appropriate.

  4. Abhinav Tripathi author

    the error message should be something like "pyexec: Exception encountered while trying to throw exception".

    Sorry I didn't explain clearly. After this PR is merged, pyexec ("raise NameError('oops')") will be caught as exception and the error text will be displayed as "pyexec: NameError: 'oops'"
    .
    The code will NOT reach that last catch in such cases now. Previously there was exception while extracting the text from exception message so the execution went into that last catch.
    I couldn't find a case currently when execution will go into that last catch but I left it for completeness.
    The error text in that case could be something to say that some exception was caught but the message could not be extracted!?
    .

    But why (error_already_set const &)? If this is some emergency catch-all, why isn't it just catch?

    Well, As far as I remember, all the python exceptions are raised in C++ (by the python api) as error_already_set. Also, the boost functions that we are using here do the same. So, the code in the try block can only raise this exception. So, basically it is same as doing catch(...) for catching every exception.
    .

    You've got pyexec in the error text for all them

    Oops. Sorry, I blindly pasted that line in all the files. I'll edit the text and update this commit.

    1. Colin Macdonald

      Thanks, that was indeed my understanding. And note my suggested error text for this wtf-this-can't-happen case was: "pyexec: Exception encountered while trying to throw exception".

      If the "extracting error text" bit was not protected by try-catch, would it just die/segfault/etc? That would probably be ok with me too.

      Anyway, let's put the try-catch inside your new function: that will be clearer. I don't need the error message to specify "pycall/pyexec/pyeval", especially since this shouldn't ever happen.

      1. Abhinav Tripathi author

        If the "extracting error text" bit was not protected by try-catch, would it just die/segfault/etc?

        I would say it may or may not segfault on different systems. Because it will be similar situation like the issue #16 which caused segfault on my system (but not on your!?)...
        .

        Anyway, let's put the try-catch inside your new function

        Done. Also, I changed things a bit to show error message with "pycall/pyexec/pyeval". I hope it is ok.

  5. Colin Macdonald

    Ok, I like this much better now.

    The extracting of the error message seems complicated: but sounds like it was non-trivial so maybe it has to be that way. I'll let Mike comment on that.

    What happens to the stack trace? I looks like it is not shown (?). Should we do PyErr_Print (); before calling fetch_exception_message? Or, if the stack trace wasn't shown before, then let's file a new bug for that...

    1. Mike Miller repo owner

      If we want to preserve the full Python stack trace we could add that as an optional feature. I like that this is just displaying the exception name and text in a one-liner.

  6. Mike Miller repo owner

    I like this, good refactoring and figuring out how to extract the relevant part of the error and format it into an Octave error. Merging, thank you.

  7. Mike Miller repo owner

    Abhinav Tripathi see 0deddd1 for some cleanup I added to this commit. Some notes:

    • The commit message for a new function can be something simple like exceptions.{cc,h} (pytave::fetch_exception_message): New function.
    • The doc string for traceback.format_exception_only says the returned list contains newline-terminated strings, so they should just be joined with an empty string and the last newline stripped off.
  8. Abhinav Tripathi author

    I like this, good refactoring and figuring out how to extract the relevant part of the error and format it into an Octave error ... see 0deddd1 for some cleanup I added to this commit

    Thanks.

    The commit message for a new function can be something simple

    Great! Will remember in future.

    they should just be joined with an empty string and the last newline stripped off.

    Oh sorry. I didn't read the official docs and just saw these functions on some other site. So, I used them. I will remember in future to read the docs once before using any function.