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 UnboundLocalError when aiohttp server raises a CancelledError #356

Merged
merged 3 commits into from
Oct 17, 2022

Conversation

armisael
Copy link
Contributor

Issue #, if available: N/A

Description of changes: In aiohttp middleware, catch BaseException instead of Exception to avoid UnboundLocalError.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@armisael armisael requested a review from a team as a code owner September 26, 2022 14:39
@armisael armisael force-pushed the bugfix/unbound-local-error branch from 3244ac9 to 3baf8ed Compare September 27, 2022 06:31
Copy link
Contributor

@carolabadeer carolabadeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. Can you please provide a code snippet that causes an UnboundLocalError to be raised just for reference?

@armisael
Copy link
Contributor Author

armisael commented Oct 4, 2022

I'm not sure, unfortunately. We have a proxy to some external services, to serve as a cache; such proxy uses aiohttp, and connects to both REST (with httpx) and SOAP (with zeep) services; we recently upgraded from python 3.6 to 3.10 and this error happened once; it's still not clear to me what exactly caused the CancelledError.

Copy link
Contributor

@carolabadeer carolabadeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged main into this branch and now some CI tests are failing - can you please take a look?

@srprash
Copy link
Contributor

srprash commented Oct 17, 2022

@carolabadeer The tests are failing for a separate issue regarding flask-sqlalchemy.
I have a PR to fix the CI which needs to be merged before any other PR.
#360

@carolabadeer
Copy link
Contributor

@srprash thank you for pointing that out! Looks good to merge now that the issue is fixed.
@armisael sorry for the confusion

@carolabadeer carolabadeer merged commit cd0fed7 into aws:master Oct 17, 2022
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.

3 participants