An exception in StreamContextManager.__aclose__() overrides the original

[adamhooper]

Checklist

• [x] The bug is reproducible against the latest release and/or master.
• [x] There are no similar issues or pull requests to fix it yet.

Describe the bug

I alluded to this in #1463. That issue is about the type of exception being raised; this bug is about the source of the exception being raised.

In this code block:

async with httpx_client.stream("GET", url) as response:
    async for blob in response.aiter_raw():
        pass
        # ... Now, imagine `aiter_raw()` raises an exception...
# ... and imagine `response.aclose()` raises an exception within `__aclose__()`.

The exception from aiter_raw() is caught by __aclose__(); the exception from response.aclose() is the one I see.

To reproduce

See #1463 for example code that causes a "Connection reset" error within the aiter_raw() block.

Expected behavior

I expect to see a stack trace with aiter_raw() in it, because aiter_raw() caused an exception. (Sure, response.aclose() also throws an exception; but as a user, do I really care?)

Actual behavior

The stack trace is for the error in response.aclose(). aiter_raw() doesn't appear in it.

Debugging material

See #1463

Environment

• OS: Linux
• Python version: 3.8.7
• HTTPX version: 0.16.1
• Async environment: asyncio
• HTTP proxy: no
• Custom certificates: no

Additional context

As a workaround, this code block will raise the correct exception:

request = client.build_request("GET", f"http://{host}:{port}")
response = await client.send(request, stream=True)
try:
    async for blob in response.aiter_raw():
        pass
except Exception as err:
    try:
        await response.aclose()
    finally:
        raise err

... this uses the inside knowledge that aiter_raw() calls aclose() to handle these three cases:

• Exception while streaming and exception from aclose(): ignore the exception from aclose() and raise the one from aiter_raw().
• Exception while streaming and success from aclose(): raise the exception from aiter_raw().
• Streaming succeeds but exception in aiter_raw()'s call to aclose(): we'll call aclose() again in our exception handler; it will no-op (because it's a second invocation); and we'll raise the exception from the original call to aclose().

I've been using Python for decades, and I feel very uncomfortable nesting exception handlers like this.

[florimondmanca]

@adamhooper I'm not sure yet I fully understood the problem here.

Here's a sample script to try and simulate the situation I think you're referring to:

import httpx
import httpcore
import trio


class BrokenStream:
    def __iter__(self):
        yield b""
        raise httpcore.ReadError("Stream iterator")

    async def __aiter__(self):
        yield b""
        raise httpcore.ReadError("Stream iterator")

    def close(self):
        raise httpcore.CloseError("Closing")

    async def aclose(self):
        raise httpcore.CloseError("Closing")


def app(request):
    return httpx.Response(200, stream=BrokenStream())


transport = httpx.MockTransport(app)
url = "https://example.org"


# Comment this out to let the async main run.
with httpx.Client(transport=transport) as client:
    with client.stream("GET", url) as response:
        for _ in response.iter_raw():
            pass


@trio.run
async def main():
    async with httpx.AsyncClient(transport=transport) as client:
        async with client.stream("GET", url) as response:
            async for _ in response.aiter_raw():
                pass

The output from this is:

Sync:

Traceback (most recent call last):
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_exceptions.py", line 326, in map_exceptions
    yield
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_models.py", line 1234, in iter_raw
    for raw_stream_bytes in self.stream:
  File "_debug/script.py", line 10, in __iter__
    raise httpcore.ReadError("Stream iterator")
httpcore.ReadError: Stream iterator

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "_debug/script.py", line 24, in <module>
    for _ in response.iter_raw():
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_models.py", line 1237, in iter_raw
    yield chunk
  File "/Users/florimond.manca/.pyenv/versions/3.8.5/lib/python3.8/contextlib.py", line 131, in __exit__
    self.gen.throw(type, value, traceback)
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_exceptions.py", line 343, in map_exceptions
    raise mapped_exc(message, **kwargs) from exc  # type: ignore
httpx.ReadError: Stream iterator

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "_debug/script.py", line 25, in <module>
    pass
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_client.py", line 1814, in __exit__
    self.response.close()
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_models.py", line 1252, in close
    self._on_close(self)
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_client.py", line 867, in on_close
    stream.close()
  File "_debug/script.py", line 13, in close
    raise httpcore.CloseError("Closing")
httpcore.CloseError: Closing

Async:

Traceback (most recent call last):
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_exceptions.py", line 326, in map_exceptions
    yield
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_models.py", line 1329, in aiter_raw
    async for raw_stream_bytes in self.stream:
  File "_debug/script.py", line 13, in __aiter__
    raise httpcore.ReadError("Stream iterator")
httpcore.ReadError: Stream iterator

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "_debug/script.py", line 41, in main
    async for _ in response.aiter_raw():
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_models.py", line 1332, in aiter_raw
    yield chunk
  File "/Users/florimond.manca/.pyenv/versions/3.8.5/lib/python3.8/contextlib.py", line 131, in __exit__
    self.gen.throw(type, value, traceback)
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_exceptions.py", line 343, in map_exceptions
    raise mapped_exc(message, **kwargs) from exc  # type: ignore
httpx.ReadError: Stream iterator

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "_debug/script.py", line 38, in <module>
    async def main():
  File "/Users/florimond.manca/Developer/encode-projects/httpx/venv/lib/python3.8/site-packages/trio/_core/_run.py", line 1896, in run
    raise runner.main_task_outcome.error
  File "_debug/script.py", line 42, in main
    pass
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_client.py", line 1836, in __aexit__
    await self.response.aclose()
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_models.py", line 1347, in aclose
    await self._on_close(self)
  File "/Users/florimond.manca/Developer/encode-projects/httpx/httpx/_client.py", line 1503, in on_close
    await stream.aclose()
  File "_debug/script.py", line 19, in aclose
    raise httpcore.CloseError("Closing")
httpcore.CloseError: Closing

We already have #1465 to fix the leaking httpcore.CloseError.

But from as far as I can see, we do see errors from iter_raw() as well as errors from .close(). They're included as causes. Right? What else would you expect to see?

[adamhooper]

The error comes when you want to catch the error.

@trio.run
async def main():
    async with httpx.AsyncClient(transport=transport) as client:
        try:
            async with client.stream("GET", url) as response:
                async for _ in response.aiter_raw():
                    pass
        except httpx.ReadError:
            print("The server broke while we were reading")
        except httpx.CloseError:
            print("The server broke while we were closing")

I'd expect to catch httpx.ReadError in this case, not httpx.CloseError.

[florimondmanca]

Hmm I see. Well unfortunately Python only follows a "single-exception-at-a-time" design, so you'd need to catch errors as close to where they happen as possible.

So what you could do as a workaround is…

@trio.run
async def main():
    async with httpx.AsyncClient(transport=transport) as client:
        try:
            async with client.stream("GET", url) as response:
                try:
                    async for _ in response.aiter_raw():
                        pass
                except httpx.ReadError:
                    print("The server broke while we were reading")
        except httpx.CloseError:
            print("The server broke while we were closing")

Which outputs the following, and doesn't look too bad, does it?

The server broke while we were reading
The server broke while we were closing

[adamhooper]

Sure -- there's also the workaround I mentioned above.

When should users use the approach laid out in the documentation, and when should users use one of these workarounds that raises the exception users want?

[euri10]

if this lands could be helpful

[florimondmanca]

@tomchristie Heya :-) Any thoughts on this one, or #1465?

[tomchristie]

Yup, we really ought to be handling this neatly.

We could for instance do something like this...

class ResponseStream(SyncByteStream):
    def __init__(self, httpcore_stream: httpcore.SyncByteStream):
        self._httpcore_stream = httpcore_stream

    def __iter__(self) -> typing.Iterator[bytes]:
        try:
            with map_httpcore_exceptions():
                for part in self._httpcore_stream:
                    yield part
        except Exception as exc:
            # If an exception occurs during streaming the data, then close the stream,
            # but mask any close exceptions that occur.
            try:
                self._httpcore_stream.close()
            except Exception:
                pass
            raise exc

Although it's not immediately clear to me if that's what we ought to be doing, ...or... if we should be ensuring something similar, but inside httpcore.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tomchristie]

Probably still valid thanks @stale - needs looking into.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tomchristie]

I believe this was closed by https://github.com/encode/httpcore/pull/310