Aiohttp: Raise an exception if trying to write into closed response

[asvetlov]

See https://stackoverflow.com/questions/47175297/handling-premature-client-disconnection-in-aiohttp

The problem is: await drain() does nothing because the buffer is not overflown, the buffer is not overflown because sent data are ignored and not accumulated in internal buffer.
transport.is_closing() should be used for check.

It should be fixed in payload writer, affects both client and server.

[azhpushkin]

Although, adding

if self._transport.is_closing():
    return noop()

to StreamResponse.write() (this fixes both server and client I guess)
https://github.com/aio-libs/aiohttp/blob/feaf0213b926a3f6093cbec0217bb2239b2b0f25/aiohttp/http_writer.py#L200

fixes logging of errors and trying to write to closing transport, for loop continues to execute and tries unsuccessfully to write to the closing transport, that means:

• all further iterations do nothing - useless load on server
• unable to catch closing of transport

Moreover, looks like there is no foreseen way to indicate that StreamResponse is closed or not, except self._eof, which looks too internal for me to use it from outer scope
IMO there are several ways to fix this bug:

• add raising asyncio.CancelledError, as SO author proposed. This is too ugly I guess, raising an Exceptions is this situation is just a hack. Worth mentioning, that it is possible to catch CancelledError but not possible to catch ConnectionError, for example, that makes this way even more dirty
• add property (e.g. is_closing) with mapping to self._transport.is_closing() in PayloadWriter and another property for StreamResponse to indicate that there is still opened stream (e.g. is_open), but this will create new API for this bug purpose only i guess (we need to pass response._payload_writer._transport.is_closing() to the user), that looks weird

I`m not sure what solution should be implemented, so just leave this message for further discussion

[rudeb0t]

I've worked around this issue by using checking request.protocol.transport.is_closing() before calling response.write() in my handler. It seems to work well so far in my tests. Although I am not so sure if this is the right way to go about this.

[asvetlov]

Explicit exception in response.write() if request.protocol.transport.is_closing() is least surprising behavior IMHO.

[socketpair]

@asvetlov how ot distinguish between remote cancellation (when remote side unexpectedly closes connection) and local cancellation (due to something.cancel()) ? Maybe I don't understand the subj.

[kyuupichan]

I've just discovered this raising of CancelledError is what was causing a hard-to-track down issue in my software. This violates the declared exceptions of aiohttp in its documentation, and IMO it should not be raising CancelledError! That is how I shut down my server - cancelling its tasks. Code should not be forced to distinguish between it cancelling its own tasks, and a CancelledError leaking form aiohttp. I suggest you throw a proper aiohttp defined exception instead.

[kyuupichan]

What @socketpair said

[azhpushkin]

@kyuupichan I think updating aiohttp version could fix this, as later, in #2989 exception type was changed from CancelledError to ConnectionResetError
Does this help?

[kyuupichan]

@maqquettex thanks, that would definitely help. It was a user of my software having the issue, and at the time I wasn't sure if it was aiohttp or my software with a problem, and I didn't want to ask them to upgrade in case it simply papered over a problem in my software. Once I'd tracked it down I didn't check if it was different in a more recent release. It seems 3.3 fixed this; I'll make that a requirement of my package.