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 on request body reading after sending response #2895

Closed
asvetlov opened this issue Mar 30, 2018 · 9 comments
Closed

Raise an exception on request body reading after sending response #2895

asvetlov opened this issue Mar 30, 2018 · 9 comments

Comments

@asvetlov
Copy link
Member

See #2893 for the issue motivation

@hubo1016
Copy link
Contributor

hubo1016 commented Apr 2, 2018

Should it be "after respond ends"?

@asvetlov
Copy link
Member Author

asvetlov commented Apr 3, 2018

I don't see a big difference but "after respond ends" sounds good

@hubo1016
Copy link
Contributor

hubo1016 commented Apr 4, 2018

If the POST data is not read when respond ends, what should be done? Read the entire post data to make the connection ready for the next request, or close it forcely? The post data might be very large, but forcely close the connection may break the "Connection: keep-alive" in the previous response.

A mark could be set after that to stop further reads.

@hubo1016
Copy link
Contributor

hubo1016 commented Apr 4, 2018

It should be "read the entire post data" and "do nothing". The connection cannot be forcely closed, or the response might be lost due to connection reset (client write to a closed connection resets the connection)

@asvetlov
Copy link
Member Author

asvetlov commented Apr 5, 2018

From my perspective we have two cases:

  1. POST data is not consumed by user's code but placed into StreamReader.
    Nothing to do here but forbid a fetching from StreamReader after response sending to fix a logical error.
  2. The data is not in StreamReader buffer because the data size is too big. In this case we cannot gracefully release the connection but should force-close it. Because we cannot read the whole huge body (it can take an infinite amount of time).

@hubo1016
Copy link
Contributor

hubo1016 commented Apr 5, 2018

@asvetlov after reading the code, I found it already implemented

with suppress(
asyncio.TimeoutError, asyncio.CancelledError):
while not payload.is_eof() and now < end_t:
timeout = min(end_t - now, lingering_time)
with CeilTimeout(timeout, loop=loop):
# read and ignore
await payload.readany()
now = loop.time()
# if payload still uncompleted
if not payload.is_eof() and not self._force_close:
self.log_debug('Uncompleted request.')
self.close()

Seems that it reads for lingering_time (by default 10s) and forcely close the connection after that. This is a reasonable solution.

@asvetlov
Copy link
Member Author

asvetlov commented Apr 5, 2018

Yes. The only thing is left: close payload as well. Now it is possible to read payload (StreamReader) data after sending response, which is weird and error prone. I thing request.content.set_exception() should be enough.

@asvetlov
Copy link
Member Author

Fixed by #2927

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants