Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.2k
gh-83371: handle exceptions from callbacks in process pools#131813
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…cess poolsUser-supplied callbacks are called from an internal pool management thread. Atpresent any exceptions they raise are not caught and so propagate out and killthe thread. This then causes problems for subsequent pool operations, includingjoining the pool hanging.As a QoL improvement, catch and handle any such exceptions using the systemexception hook. Thus by default details of the exception will be printed tostderr, but the pool's integrity will remain intact.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
well currently unhandled exceptions from results do go somewhere, threading.excepthook you could call that instead? here's an alternative approach |
Lib/multiprocessing/pool.py Outdated
| try: | ||
| return callback(args) | ||
| except Exception as e: | ||
| sys.excepthook(*sys.exc_info()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybe threading.excepthook would be better to call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, quite possibly. In fact I did use that in an earlier version of the code. I decided to change because the thread is internal to the pool code, which does not set itsexcepthook. So ISTM it will always end up using the system hook anyway, and the code is a little uglier because it needs to marshal the args then explicitlydel them to avoid reference cycles:
except Exception as e: args = threading.ExceptHookArgs([type(e), e, e.__traceback__, None]) threading.excepthook(args) del argsHowever, it probably is more robust/future-proof/correct to usethreading.excepthook, so I'm happy to switch to this if you think it is appropriate.
…sing themwith the default exception hook.This is a breaking change to the API, but only in cases where the existing codewould be completely broken anyway, so hopefully it isn't a problem.TODO: docs need updating
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
sub-test and use a barrier to ensure it is reliable. We must ensure all tasksare submitted before any of them run and mark the pool as broken. Sincebarriers can't be shared between processes, do not run that sub-test withprocess-based parallelism.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
…esources.Attempt to fix that by not creating the CallbackError inside the except clausewhere the exceptions that cause them are caught, but just storing thoseexceptions and creating the exception group later when required.Also ensure if we are exiting a pool's context manager due to a BrokenPoolErrorthat we don't re-raise it with the same error added again.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
All commit authors signed the Contributor License Agreement. |
…le: tryharder to collect them. This doesn't seem like it should be necessary or make adifference, but let's give it a try...
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
…epthook"This reverts this branch back to just calling `threading.excepthook`, insteadof trying to raise the exception when the user gets the result or exits thecontext manager. I would like to get the immediate bug fixed, and I think thisis a simple, low-risk fix that does that.From what I understand it isn't possible to prevent the reference cycle whichis causing all those test failures. We need to store the exception to raise itlater, which implicitly creates one.We can always add raising the exception as an additional improvement later, butin the meantime if any user code wants to handle the exceptions it can do so byinstalling a default exception handler.
I am going to revert the branch back to just calling From what I understand it isn't possible to prevent the reference cycle which is causing all those test failures. We need to store the exception to raise it later, which implicitly creates one. We can always add raising the exception as an additional improvement later, but in the meantime if any user code wants to handle the exceptions it can do so by installing a default exception handler. |
Uh oh!
There was an error while loading.Please reload this page.
User-supplied callbacks are called from an internal pool management thread. At present any exceptions they raise are not caught and so propagate out and kill the thread. This then causes problems for subsequent pool operations, including joining the pool hanging.
As a QoL improvement, catch and handle any such exceptions using the system exception hook. Thus by default details of the exception will be printed to stderr, but the pool's integrity will remain intact.