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

Bug: return in finally can swallow exceptions #7599

Open
iritkatriel opened this issue Oct 22, 2024 · 3 comments
Open

Bug: return in finally can swallow exceptions #7599

iritkatriel opened this issue Oct 22, 2024 · 3 comments
Labels
area/local/start-api sam local start-api command blocked/more-info-needed More info is needed from the requester. If no response in 14 days, it will become stale.

Comments

@iritkatriel
Copy link

At this location:

return auth_service_error

there is a return statement in a finally block, which would swallow any in-flight exception.

This means that if a BaseException (such as KeyboardInterrupt) is raised from the try body, or any exception is raised from an except: clause, it will not propagate on as expected.

See also https://docs.python.org/3/tutorial/errors.html#defining-clean-up-actions.

@iritkatriel iritkatriel added the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Oct 22, 2024
@lucashuy
Copy link
Contributor

Thanks for opening this issue, this might be by design since this control block is within the Flask app, and an exception raised within this block will halt the entire sam local start-api command, which is supposed to be left open in the background.

The return only triggers if we catch a specific, known, Lambda execution error. The catch all should still be raised/re-raised normally since we aren't creating a Flask service exception in the catch all block.

Was there a specific exception or behaviour that you saw during sam local start-api that was swallowed, but shouldn't have?

@lucashuy lucashuy added blocked/more-info-needed More info is needed from the requester. If no response in 14 days, it will become stale. area/local/start-api sam local start-api command and removed stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Oct 24, 2024
@iritkatriel
Copy link
Author

Was there a specific exception or behaviour that you saw during sam local start-api that was swallowed, but shouldn't have?

No, my comment is based on inspection of the code.

I see what you mean - perhaps it's worth adding a comment stating this is deliberate, in case someone else has my reaction.

@iritkatriel
Copy link
Author

iritkatriel commented Oct 24, 2024

On second thought - you don't expect there to be an in-flight exception when the return executes, so perhaps just dedent the

            if auth_service_error:
                return auth_service_error

so that it's not in the finally?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/local/start-api sam local start-api command blocked/more-info-needed More info is needed from the requester. If no response in 14 days, it will become stale.
Projects
None yet
Development

No branches or pull requests

2 participants