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

Cannot cancel reading from incoming connection and retry later #363

Closed
saghul opened this issue May 15, 2015 · 5 comments
Closed

Cannot cancel reading from incoming connection and retry later #363

saghul opened this issue May 15, 2015 · 5 comments
Labels

Comments

@saghul
Copy link
Contributor

saghul commented May 15, 2015

In my application I'm doing something like this:

  • Try to receive data
  • Under certain conditions cancel() the task
  • Try to receive data again

This yields the following assertion error:

  File "/Users/saghul/.virtualenvs/cpython342/lib/python3.4/site-packages/aiohttp/web_ws.py", line 206, in receive
    msg = yield from self._reader.read()
  File "/Users/saghul/.virtualenvs/cpython342/lib/python3.4/site-packages/aiohttp/streams.py", line 501, in read
    result = yield from super().read()
  File "/Users/saghul/.virtualenvs/cpython342/lib/python3.4/site-packages/aiohttp/streams.py", line 362, in read
    assert not self._waiter

The problem looks quite obvious, in DataQueue.read the waiter is not reset until feed_data is called: https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/streams.py#L364 and https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/streams.py#L343

The solution, however, is not so obvious, here is a possible solution, let me know if you like the approach and I'll submit a PR with it:

diff --git a/aiohttp/streams.py b/aiohttp/streams.py
index ce0cce4..3879ee6 100644
--- a/aiohttp/streams.py
+++ b/aiohttp/streams.py
@@ -361,7 +361,11 @@ class DataQueue:

             assert not self._waiter
             self._waiter = asyncio.Future(loop=self._loop)
-            yield from self._waiter
+            try:
+                yield from self._waiter
+            except (asyncio.CancelledError, asyncio.TimeoutError):
+                self._waiter = None
+                raise

         if self._buffer:
             data, size = self._buffer.popleft()

For now I'm setting ws._reader._waiter to None after cancelling the task , which is quite ugly... :-S

@saghul saghul changed the title Canno cancel reading from incoming connection and retry later Cannot cancel reading from incoming connection and retry later May 15, 2015
@ludovic-gasc
Copy link
Contributor

For my understanding, could you describe an use case of this need? What's the interest?
Is it for a WebSockets context?

@saghul
Copy link
Contributor Author

saghul commented May 15, 2015

Here is my usecase in more detail, thanks for your interest @GMLudo!

My application is this one: https://github.com/saghul/CallRoulette

In a nutshell, if a user connects, it waits until a new user connects and when we have 2, we start processing messages between the 2.

Now, it's possible that while I wait for the second user to connect, the first one disconnects. In order to know about this, the only way is to use the receive() coroutine, AFAIK, together with my other coroutine, which waits for the second connection.

Once the 2nd connection comes, I want to cancel the receive() I started and start the processing.

In a nutshell, this looks like it was planned, there is even a related test about it, but it doesn't test the actual retrying. This also happens if you want to receive data with a timeout, AFAIS.

@kxepal
Copy link
Member

kxepal commented May 15, 2015

Similar issue happens when user requests some URL that served by aiohttp with browser and hits Stop button / Esc in order to cancel page load (he tired to wait for the response, he change his mind etc.) - browser successfully terminates connection, but such assertion error occurs in logs.

Things goes wild when you have users with unstable connection (mobile ones) - your logs are full of such assertions.

@fafhrd91
Copy link
Member

@saghul change looks good, but it hard to tell if it safe. Usually we treat cancelation as disconnection. We should just cearfuly test this change. I can test it on one of my production instances.

@lock
Copy link

lock bot commented Oct 29, 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.

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

No branches or pull requests

4 participants