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

Unable to StreamWriter.drain() #16

Closed
barosl opened this issue Mar 23, 2014 · 9 comments
Closed

Unable to StreamWriter.drain() #16

barosl opened this issue Mar 23, 2014 · 9 comments

Comments

@barosl
Copy link

barosl commented Mar 23, 2014

I'm constantly getting the following exception:

Future/Task exception was never retrieved:
Traceback (most recent call last):
  File "/home/lolgall/venv/lib/python3.3/site-packages/asyncio/tasks.py", line 283, in _step
    result = next(coro)
  File "/home/lolgall/venv/lib/python3.3/site-packages/websockets/protocol.py", line 170, in send
    yield from self.write_frame(opcode, data)
  File "/home/lolgall/venv/lib/python3.3/site-packages/websockets/protocol.py", line 313, in write_frame
    yield from self.writer.drain()
  File "/home/lolgall/venv/lib/python3.3/site-packages/asyncio/streams.py", line 287, in drain
    return self._protocol._make_drain_waiter()
  File "/home/lolgall/venv/lib/python3.3/site-packages/asyncio/streams.py", line 184, in _make_drain_waiter
    assert waiter is None or waiter.cancelled()
AssertionError

What would be the reason?

@aaugustin
Copy link
Member

I have no idea. Unfortunately, my crystal ball is momentarily out of order, so I can't read the code you have carefully omitted from this bug report.

@aaugustin
Copy link
Member

Regardless of how you're running into this problem, make sure you're using the latest version of asyncio or try your code with Python 3.4, as some bugs have been fixed recently in this area.

@ghost
Copy link

ghost commented Mar 28, 2017

For information, I came across this problem using websockets==3.2 while I was trying to use many "await websocket.send(data)" in parallel. WebSocketCommonProtocol.send() calls asyncio.StreamWriter.drain() which apparently must not be called twice in parallel.

I was able to reproduce it without websockets and opened an issue here: http://bugs.python.org/issue29930
I think it might be interesting to follow the evolution of this issue and make changes in websockets if necessary.

@aaugustin

@aaugustin
Copy link
Member

Thanks @metathink, great investatigation!

@aaugustin aaugustin reopened this Mar 28, 2017
@RemiCardona
Copy link
Collaborator

I'm glad to see this being reopened. I've been investigating this on and off for the past 2 weeks and came to the same conclusion: drain is nowhere near "thread"/task safe. And because websockets may call write_frame independently (IOW: in another Task) of user code through the PONG reply, there's a chance things may break.

I've been a able to reliably force the bug using https://github.com/mariusae/trickle (with a few local fixes to make it work with CLOEXEC sockets that python uses) to slow down network write speeds, which forces the sender to fill up the write buffer, thus revealing the bug.

The patch above simply adds a lock around drain() and I haven't been able to get this code to fail.

I've also been reading njs's "post async/await" post and the more I think about it, a module like websocket (and many others, really) could probably do away with all the whole transports._FlowControlMixin: writes could/should be coroutines that "block" until frame data has been flushed to kernel/libc buffers. The userspace/asyncio buffering is counterproductive. The drain fail being just the tip of the iceberg. But that's work for another PR.

@aaugustin
Copy link
Member

Thanks for the patch, let's do that. Even if the issue eventually gets fixed in Python, we need to work around it in versions of Python where it exists.

@aaugustin
Copy link
Member

See #170 for the more general considerations.

@aaugustin
Copy link
Member

@RemiCardona Once you merge your patch, it would be a good time to make a release, wouldn't it?

@aaugustin
Copy link
Member

I released version 3.3 with this change.

RemiCardona added a commit to Polyconseil/aioamqp that referenced this issue Apr 10, 2017
As of CPython 3.6.1, drain() is not coroutine-safe so put a lock around it
to make sure it only ever gets called from a single coroutine at a time.

http://bugs.python.org/issue29930
python-websockets/websockets#16
RemiCardona added a commit to Polyconseil/aioamqp that referenced this issue Apr 10, 2017
As of CPython 3.6.1, drain() is not coroutine-safe so put a lock around it
to make sure it only ever gets called from a single coroutine at a time.

http://bugs.python.org/issue29930
python-websockets/websockets#16
RemiCardona added a commit to Polyconseil/aioamqp that referenced this issue Apr 13, 2017
As of CPython 3.6.1, drain() is not coroutine-safe so put a lock around it
to make sure it only ever gets called from a single coroutine at a time.

http://bugs.python.org/issue29930
python-websockets/websockets#16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants