Skip to content

Call out that teardown functions must never raise #1767

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.

Sign up for GitHub

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

Jump to bottom
Closed
TallGirlVanessa wants to merge 1 commit into from
Closed

Call out that teardown functions must never raise #1767

TallGirlVanessa wants to merge 1 commit into from

Conversation

@TallGirlVanessa
Copy link

@TallGirlVanessa TallGirlVanessa commented Apr 4, 2016

Also add a more detailed explanation of how the request context and app
context interact.

Re-open of #1451, but against master.

jeffwidman
jeffwidman reviewed Apr 4, 2016
View reviewed changes
docs/reqcontext.rst Outdated

However, if a request context is pushed and there's already an application
context (for the request context's app) on the stack, the request context
won't* push a new app context but will reuse the existing one. The request

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@TallGirlVanessa
Call out that teardown functions must never raise
740bc3f 
Also add a more detailed explanation of how the request context and app
context interact.
@jeffwidman jeffwidman mentioned this pull request Apr 5, 2016
Closed
@davidism
Copy link
Member

davidism commented Apr 5, 2016

Let's have the discussion in comments rather than review line notes.

@TallGirlVanessa
Copy link
Author

TallGirlVanessa commented Apr 6, 2016

Sounds good. Summarizing:

  • This PR expands and clarifies documentation around teardown functions, including a stronger warning that users must ensure teardowns don't raise, even teardowns registered by Flask extensions.
  • @ThiefMaster pointed out that the warning seemed off-putting for new users.
  • @jeffwidman and I agreed: paraphrasing, the warning sounds less like an explanation of a feature and more like a workaround for a bug. That said, maybe there is a bug, and maybe the right thing to do is fix it: in other words, maybe Flask should handle teardown exceptions in a more robust way.
  • @jeffwidman is +1 on merging this now as long as we make a new issue to track improved handling of teardown exceptions, and asked @ThiefMaster for thoughts.
  • @untitaker asked @mitsuhiko for the reasoning behind the current behavior.

mitsuhiko added a commit that referenced this pull request May 26, 2016
@mitsuhiko
Improve application context popping
8482ce6 
Exceptions during teardown handling will no longer leave application
contexts lingering around.  This fixes #1767
@mitsuhiko mitsuhiko mentioned this pull request May 26, 2016
Merged
@mitsuhiko
Copy link
Contributor

mitsuhiko commented May 26, 2016

Instead of improving the docs i will improve the behavior instead :)

@mitsuhiko mitsuhiko closed this May 26, 2016
@TallGirlVanessa TallGirlVanessa deleted the update_teardown_docs branch October 18, 2018 14:36
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@mitsuhiko mitsuhiko

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@TallGirlVanessa @davidism @mitsuhiko @ThiefMaster @jeffwidman @untitaker