Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segfault/Assertion when an exception is thrown in Propagator::undo(). #53

Closed
rkaminsk opened this issue Feb 4, 2020 · 5 comments
Closed

Comments

@rkaminsk
Copy link
Member

rkaminsk commented Feb 4, 2020

This is not a severe issue because usually the undo function does not throw exceptions but when developing python code this easily leads to gobbled up error messages. There is either a segfault or and assertion if the program in bug.cc.gz throws an exception in undo.

@BenKaufmann
Copy link
Contributor

What's your suggestion here? Imo all callbacks (i.e. all functions in the AbstractXXX-Interfaces) should be noexcept. To me the only sensible thing to do in case of an exception from one of these callbacks is to call std::terminate so that the user can fix the problem. Another alternative on clasp's side would be to swallow the exception and continue as if nothing happened. But this seems to be a horrible strategy to me. Finally, properly handling the exception in clasp is basically out of question as all solving related code is deliberately not exception safe (mostly for performance reasons).

@rkaminsk
Copy link
Member Author

rkaminsk commented Feb 7, 2020

If an exception is thrown in propagate (in the c++ part), then it just comes out in solve again (even with threads). I was using this behavior to implement nice error reporting. I was also assuming that the only valid thing to do with a control object would be to delete it afterward. If this is not possible with undo (or other functions), then it is better if I call terminate myself to at least report the last error.

@BenKaufmann
Copy link
Contributor

For propagate() and check() this typically works but only under the assumption that we can do a clean "rollback" to the top-level. That is, a noexcept undo() is a core requirement for the whole mechanism (similar to noexcept destructors).

If you want, I'll wrap the undo() call and do a std::terminate() in case of an exception. Otherwise, I would simply close this issue.

@rkaminsk
Copy link
Member Author

rkaminsk commented Feb 8, 2020

For propagate() and check() this typically works but only under the assumption that we can do a clean "rollback" to the top-level. That is, a noexcept undo() is a core requirement for the whole mechanism (similar to noexcept destructors).

If you want, I'll wrap the undo() call and do a std::terminate() in case of an exception. Otherwise, I would simply close this issue.

I can take care of it. There is a single point where I have to do it. No matter where terminate is called, I also have to change some code how errors are handled in the bindings. But I already did something like this for the finalize callback. Thanks for looking into it!

@rkaminsk
Copy link
Member Author

rkaminsk commented Feb 8, 2020

I think you can just close this. There is no point to do anything on your end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants