Repository: falconry/falcon

Issue:
[ASGI] close() not called if iterating over resp.stream raises an exception
At the time of writing, the `close()` method of [`resp.stream`](https://falcon.readthedocs.io/en/stable/api/request_and_response_asgi.html#falcon.asgi.Response.stream) is only called if the provided generator is successfully exhausted (or yields `None`).

However, the primary use case of the `close()` method is freeing up resources, so one might expect the method to be called regardless. It is also what [PEP 3333 compliant](https://www.python.org/dev/peps/pep-3333/#specification-details) WSGI servers do.

I would suggest introducing a `finally` clause, and moving the invocation of `close()` there.

Make sure to also write proper regression tests:
>  Kurt Griffiths @kgriffs
> We also need a test that forces stream to raise an error.
> (and ensures close() is called)


Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1949?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) Report
> Merging [#1949](https://codecov.io/gh/falconry/falcon/pull/1949?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) (7450776) into [master](https://codecov.io/gh/falconry/falcon/commit/e255bff9ae5a90d0cb3fe9af7c16917f18a92dc3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) (e255bff) will **decrease** coverage by `32.16%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1949/graphs/tree.svg?width=650&height=150&src=pr&token=zOtkfNQ3h6&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry)](https://codecov.io/gh/falconry/falcon/pull/1949?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry)

```diff
@@             Coverage Diff              @@
##            master    #1949       +/-   ##
============================================
- Coverage   100.00%   67.83%   -32.17%     
============================================
  Files           63       63               
  Lines         6610     6610               
  Branches      1067     1068        +1     
============================================
- Hits          6610     4484     -2126     
- Misses           0     2108     +2108     
- Partials         0       18       +18     
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1949?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) | Coverage Δ | |
|---|---|---|
| [falcon/asgi/app.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvYXBwLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/ws.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvd3MucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/reader.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvcmVhZGVyLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/stream.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvc3RyZWFtLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/request.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvcmVxdWVzdC5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/response.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvcmVzcG9uc2UucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/multipart.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvbXVsdGlwYXJ0LnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/structures.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvc3RydWN0dXJlcy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/\_asgi\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvX2FzZ2lfaGVscGVycy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/\_request\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry#diff-ZmFsY29uL2FzZ2kvX3JlcXVlc3RfaGVscGVycy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [34 more](https://codecov.io/gh/falconry/falcon/pull/1949/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1949?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1949?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry). Last update [e255bff...7450776](https://codecov.io/gh/falconry/falcon/pull/1949?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=falconry).

dimucciojonathan: @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: 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: 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: 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 this?
vytas7: 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.

Patch:
diff --git a/docs/_newsfragments/1943.bugfix.rst b/docs/_newsfragments/1943.bugfix.rst
new file mode 100644
index 0000000..d313689
--- /dev/null
+++ b/docs/_newsfragments/1943.bugfix.rst
@@ -0,0 +1,3 @@
+If provided, the ``close()`` method of an ASGI
+:attr:`resp.stream <falcon.asgi.Response.stream>` is now guaranteed to be
+called even in the case of an exception raised while iterating over the data.
diff --git a/falcon/asgi/app.py b/falcon/asgi/app.py
index 2b8dc07..922aaab 100644
--- a/falcon/asgi/app.py
+++ b/falcon/asgi/app.py
@@ -651,19 +651,24 @@ class App(falcon.app.App):
             #
 
             if hasattr(stream, 'read'):
-                while True:
-                    data = await stream.read(self._STREAM_BLOCK_SIZE)
-                    if data == b'':
-                        break
-                    else:
-                        await send(
-                            {
-                                'type': EventType.HTTP_RESPONSE_BODY,
-                                # NOTE(kgriffs): Handle the case in which data == None
-                                'body': data or b'',
-                                'more_body': True,
-                            }
-                        )
+                try:
+                    while True:
+                        data = await stream.read(self._STREAM_BLOCK_SIZE)
+                        if data == b'':
+                            break
+                        else:
+                            await send(
+                                {
+                                    'type': EventType.HTTP_RESPONSE_BODY,
+                                    # NOTE(kgriffs): Handle the case in which
+                                    #   data is None
+                                    'body': data or b'',
+                                    'more_body': True,
+                                }
+                            )
+                finally:
+                    if hasattr(stream, 'close'):
+                        await stream.close()
             else:
                 # NOTE(kgriffs): Works for both async generators and iterators
                 try:
@@ -698,9 +703,12 @@ class App(falcon.app.App):
                         '__aiter__ method. Error raised while iterating over '
                         'Response.stream: ' + str(ex)
                     )
-
-            if hasattr(stream, 'close'):
-                await stream.close()
+                finally:
+                    # NOTE(vytas): This could be DRYed with the above identical
+                    #   twoliner in a one large block, but OTOH we would be
+                    #   unable to reuse the current try.. except.
+                    if hasattr(stream, 'close'):
+                        await stream.close()
 
         await send(_EVT_RESP_EOF)
 


Test patch:
diff --git a/tests/asgi/test_hello_asgi.py b/tests/asgi/test_hello_asgi.py
index 11e4233..c60cc67 100644
--- a/tests/asgi/test_hello_asgi.py
+++ b/tests/asgi/test_hello_asgi.py
@@ -107,6 +107,47 @@ class ClosingFilelikeHelloResource:
         resp.set_stream(self.stream, self.stream_len)
 
 
+class ClosingStreamResource:
+    class Emitter:
+        def __init__(self, value, divisor):
+            self._value = value
+            self._divisor = divisor
+            self._remainder = None
+
+            self.closed = False
+
+        async def close(self):
+            self.closed = True
+
+        def __aiter__(self):
+            if self._remainder is None:
+                quotient, self._remainder = divmod(self._value, self._divisor)
+                assert quotient >= 0
+
+            return self
+
+        async def __anext__(self):
+            if self._value == 0:
+                raise StopAsyncIteration
+
+            if self._value >= self._divisor:
+                self._value -= self._divisor
+                return f'{self._divisor}\n'.encode()
+
+            self._value = 0
+            return f'{self._remainder}\n'.encode()
+
+    def __init__(self):
+        self.stream = None
+
+    async def on_get(self, req, resp):
+        self.stream = None
+
+        value = req.get_param_as_int('value', default=10)
+        divisor = req.get_param_as_int('divisor', default=3)
+        self.stream = resp.stream = self.Emitter(value, divisor)
+
+
 class AIOFilesHelloResource:
     def __init__(self):
         self.sample_utf8 = testing.rand_string(8 * SIZE_1_KB, 16 * SIZE_1_KB).encode()
@@ -297,6 +338,33 @@ class TestHelloWorld:
         assert actual_len == expected_len
         assert len(result.content) == expected_len
 
+    @pytest.mark.parametrize(
+        'value,divisor,text,error',
+        [
+            (10, 3, '3\n3\n3\n1\n', None),
+            (10, 7, '7\n3\n', None),
+            (10, 17, '10\n', None),
+            (20, 0, '', ZeroDivisionError),
+        ],
+    )
+    def test_closing_stream(self, client, value, divisor, text, error):
+        resource = ClosingStreamResource()
+        client.app.add_route('/stream', resource)
+
+        if error:
+            with pytest.raises(error):
+                client.simulate_get(
+                    '/stream', params={'value': value, 'divisor': divisor}
+                )
+        else:
+            result = client.simulate_get(
+                '/stream', params={'value': value, 'divisor': divisor}
+            )
+            assert result.status_code == 200
+            assert result.text == text
+
+        assert resource.stream.closed
+
     def test_status_not_set(self, client):
         client.app.add_route('/nostatus', NoStatusResource())