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

Wrapping a function that raises a BaseException (not an Exception) can indefinitely wedge all future calls using that argument list #35

Closed
charles-dyfis-net opened this issue Aug 22, 2024 · 3 comments · Fixed by #37

Comments

@charles-dyfis-net
Copy link
Contributor

Because refresh calls mark_being_updated() before it invokes value_future_provider() or await value_future, any failure in those calls that doesn't go through the existing exception handlers never calls mark_update_aborted(), and thus leaves a future that will never be populated in the status tracker.

I have a fix for this (incorporated in https://github.com/charles-dyfis-net/memoize/tree/local), but it lives in a tree that makes other heavy-handed changes which may or may not be acceptable upstream.

@charles-dyfis-net
Copy link
Contributor Author

Note that this can happen even for code that doesn't ever explicitly raise a non-Exception throwable: CancelledError in recent versions of Python (since at least 3.8) inherits directly from BaseException; see this change discussed in https://bugs.python.org/issue32528

Thus, for any web framework that cancels coprocesses generating for pages where the client disconnects, memoize-wrapped functions can get wedged when using the upstream tree.

@zmumi
Copy link
Member

zmumi commented Oct 4, 2024

Good point about CancelledError inheriting from BaseException since 3.8. Still other BaseExceptions like GeneratorExit, KeyboardInterrupt, or SystemExit (see https://docs.python.org/3/library/exceptions.html#exception-hierarchy) IMO should not be caught, as they are related to process termination and we should not disrupt that. So I'd proceed to explicitly catch CancelledError only

zmumi added a commit that referenced this issue Oct 4, 2024
… than Exception, so we need to catch it explicitly.
zmumi added a commit that referenced this issue Oct 7, 2024
#37)

* [#35] Since 3.8, CancelledError is a subclass of BaseException rather than Exception, so we need to catch it explicitly.

* [#35] Enabled `mypy` for `wrapper` function (and fixed issues)

---------

Co-authored-by: Michał Żmuda <michal.zmuda@ringieraxelspringer.pl>
@zmumi zmumi closed this as completed in #37 Oct 7, 2024
@charles-dyfis-net
Copy link
Contributor Author

I no longer have a dog in this fight -- replaced py-memoize with something built to a narrower set of requirements -- but I'd generally tend to suggest catching all BaseException types and then re-throwing them; that way a parent process can choose to suppress a KeyboardInterrupt without wedging the state of a cached call that was underway.

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 a pull request may close this issue.

2 participants