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

Fix ThreadResult not handling exceptions; remove apply_e. #556

Closed
wants to merge 1 commit into from

Conversation

rwe
Copy link

@rwe rwe commented May 13, 2015

Using threadpools to do normal things like .spawn, .apply etc worked significantly differently (erroneously) compared to other pools. Specifically, they would always swallow all exceptions: you'd get an AsyncResult back that would say success and no value.

The very strange apply_e wrapper attempted to work around this by wrapping functions in a try/catch that would return the result for apply_e to re-raise. Except that all "non-expected" exceptions would be silently swallowed as successful. Thankfully(?) the only users of this used Exception and BaseException.

The correct thing is to store the exception for the AsyncResult to handle/raise/etc. as normal, which eliminates the weirdness and the danger of silently dropping exceptions.

To further clarify: by setting returning False from ThreadResult.successful and setting ThreadResult.exception member, the AsyncResult will correctly capture that exception to potentially reraise on a .get if necessary.

@rwe
Copy link
Author

rwe commented Jun 14, 2015

@denik I've rebased this against recent master and have been using the patch successfully in production. I believe the change is correct and useful; let me know if there's more I can do to increase the likelihood that this gets merged in.

@jamadden
Copy link
Member

In principle this sounds like the correct thing to do, but I am a little concerned about the backwards compatibility implications. The main concern I see is that code that didn't raise exceptions before could suddenly start raising them. I'm less concerned about the loss of apply_e and wrap_errors because they weren't documented, but that is a compatibility risk.

For those reasons, I can see this going into a 1.1, but definitely not into a 1.0.3.

Could you please update the PR to include a test case for this new behaviour and rebase one more time (the way the tests run on travis has been changed and should be more reliable)?

@jamadden jamadden closed this in b567260 Jun 30, 2015
hashbrowncipher pushed a commit to hashbrowncipher/gevent that referenced this pull request Oct 20, 2018
Report more meaningful errors on why virtualenv creation failed
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

Successfully merging this pull request may close these issues.

None yet

2 participants