Skip to content
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.

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

Raise an exception if trying to write into closed response #2499

Closed
asvetlov opened this issue Nov 10, 2017 · 8 comments
Closed

Raise an exception if trying to write into closed response #2499

asvetlov opened this issue Nov 10, 2017 · 8 comments
Milestone

Comments

@asvetlov
Copy link
Member

@asvetlov asvetlov commented Nov 10, 2017

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.

@asvetlov asvetlov added the bug label Nov 10, 2017
@azhpushkin
Copy link
Contributor

@azhpushkin azhpushkin commented Dec 6, 2017

Although, adding

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

to StreamResponse.write() (this fixes both server and client I guess)

def write(self, chunk, *, drain=True, LIMIT=64*1024):

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

Loading

@rudeb0t
Copy link

@rudeb0t rudeb0t commented Dec 18, 2017

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.

Loading

@asvetlov asvetlov added this to the 3.0 milestone Dec 18, 2017
@asvetlov
Copy link
Member Author

@asvetlov asvetlov commented Dec 18, 2017

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

Loading

@socketpair
Copy link
Contributor

@socketpair socketpair commented Feb 13, 2018

@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.

Loading

kornicameister added a commit to kornicameister/korni-stats-collector that referenced this issue Jun 7, 2018
This PR updates [aiohttp](https://pypi.org/project/aiohttp) from **3.2.1** to **3.3.1**.



<details>
  <summary>Changelog</summary>
  
  
   ### 3.3.0
   ```
   ==================

Features
--------

- Raise ``ConnectionResetError`` instead of ``CancelledError`` on trying to
  write to a closed stream. (`2499 &lt;https://github.com/aio-libs/aiohttp/pull/2499&gt;`_)
- Implement ``ClientTimeout`` class and support socket read timeout. (`2768 &lt;https://github.com/aio-libs/aiohttp/pull/2768&gt;`_)
- Enable logging when ``aiohttp.web`` is used as a program (`2956 &lt;https://github.com/aio-libs/aiohttp/pull/2956&gt;`_)
- Add canonical property to resources (`2968 &lt;https://github.com/aio-libs/aiohttp/pull/2968&gt;`_)
- Forbid reading response BODY after release (`2983 &lt;https://github.com/aio-libs/aiohttp/pull/2983&gt;`_)
- Implement base protocol class to avoid a dependency from internal
  ``asyncio.streams.FlowControlMixin`` (`2986 &lt;https://github.com/aio-libs/aiohttp/pull/2986&gt;`_)
- Cythonize ``helpers.reify``, 5% boost on macro benchmark (`2995 &lt;https://github.com/aio-libs/aiohttp/pull/2995&gt;`_)
- Optimize HTTP parser (`3015 &lt;https://github.com/aio-libs/aiohttp/pull/3015&gt;`_)
- Implement ``runner.addresses`` property. (`3036 &lt;https://github.com/aio-libs/aiohttp/pull/3036&gt;`_)
- Use ``bytearray`` instead of a list of ``bytes`` in websocket reader. It
  improves websocket message reading a little. (`3039 &lt;https://github.com/aio-libs/aiohttp/pull/3039&gt;`_)
- Remove heartbeat on closing connection on keepalive timeout. The used hack
  violates HTTP protocol. (`3041 &lt;https://github.com/aio-libs/aiohttp/pull/3041&gt;`_)
- Limit websocket message size on reading to 4 MB by default. (`3045 &lt;https://github.com/aio-libs/aiohttp/pull/3045&gt;`_)


Bugfixes
--------

- Don&#39;t reuse a connection with the same URL but different proxy/TLS settings
  (`2981 &lt;https://github.com/aio-libs/aiohttp/pull/2981&gt;`_)
- When parsing the Forwarded header, the optional port number is now preserved.
  (`3009 &lt;https://github.com/aio-libs/aiohttp/pull/3009&gt;`_)


Improved Documentation
----------------------

- Make Change Log more visible in docs (`3029 &lt;https://github.com/aio-libs/aiohttp/pull/3029&gt;`_)
- Make style and grammar improvements on the FAQ page. (`3030 &lt;https://github.com/aio-libs/aiohttp/pull/3030&gt;`_)
- Document that signal handlers should be async functions since aiohttp 3.0
  (`3032 &lt;https://github.com/aio-libs/aiohttp/pull/3032&gt;`_)


Deprecations and Removals
-------------------------

- Deprecate custom application&#39;s router. (`3021 &lt;https://github.com/aio-libs/aiohttp/pull/3021&gt;`_)


Misc
----

- 3008, 3011
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/aiohttp
  - Changelog: https://pyup.io/changelogs/aiohttp/
  - Repo: https://github.com/aio-libs/aiohttp
</details>
@kyuupichan
Copy link

@kyuupichan kyuupichan commented Apr 16, 2019

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.

Loading

@kyuupichan
Copy link

@kyuupichan kyuupichan commented Apr 16, 2019

What @socketpair said

Loading

@azhpushkin
Copy link
Contributor

@azhpushkin azhpushkin commented Apr 16, 2019

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

Loading

@kyuupichan
Copy link

@kyuupichan kyuupichan commented Apr 16, 2019

@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.

Loading

@lock lock bot added the outdated label Apr 17, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants