falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.53k stars 945 forks source link

fix(asgi): add `finally` statement for stream.close() #1949

Closed dimucciojonathan closed 2 years ago

dimucciojonathan commented 3 years ago

Summary of Changes

This PR has the same implementation from the closed PR #1948, except this is on a new branch.

Related Issues

Closes #1943

codecov[bot] commented 3 years ago

Codecov Report

Merging #1949 (01d7cd6) into master (62c8a2a) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1949   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        63           
  Lines         6678      6681    +3     
  Branches      1239      1241    +2     
=========================================
+ Hits          6678      6681    +3     
Impacted Files Coverage Δ
falcon/asgi/app.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 62c8a2a...01d7cd6. Read the comment docs.

dimucciojonathan commented 3 years ago

@vytas7

I have another beginner question / progress for #1943. When adding tests for .close() when an exception occurs, it seems like the best approach is adding assertions to the test_genfunc_error and test_nongenfunc_error functions in test_hello_asgi.py since this is where the exception is tested.

I see that .close() is tested directly under these functions, but a different resource variable is used. So if I'm on the right path here, how can I verify the HelloResource is closed?

I will also update the branch containing the PR with the change to complete bulletpoint 1 of your previous comment.

vytas7 commented 3 years ago

Hi again @dimucciojonathan! Just checking if you are planning to continue working on this issue, or could we possibly join forces with another contributor?

dimucciojonathan commented 3 years ago

Hi @vytas7 , thank you for asking. I am all out of ideas for how to properly close this issue, so feel free to assign another contributor and I will follow along!

vytas7 commented 3 years ago

Thanks for the prototyping so far @dimucciojonathan -- maybe this issue is not that trivial as it looks at first glance. For instance, in addition to what has been prototyped above, one non-obvious choice is whether to call await close() as it is now, or to schedule that later asynchronously.

@wsz you had some ideas how to resolve this, would you like to take over the issue?

vytas7 commented 2 years ago

Hi again @dimucciojonathan, I believe this is how one of the ways to fix this could look like. Still remaining are a Towncrier newsfragment, and proper unit test coverage for the change.