StreamWriter.drain() coroutine can "short-circuit"
jashandeep-sohi opened this issue · 10 comments
When using the Streams API to make servers (i.e. using loop.start_server), the StreamWriter.drain() coroutine can short-circuit into an infinite loop and stop bubbling up any exceptions to the caller. This happens, for example, when a client disconnects early: http://stackoverflow.com/questions/32175217/how-to-detect-write-failure-in-asyncio/
A simple solution for the user is to just do an empty yield before every drain() call, but perhaps this fix should be moved upstream.
This is an interesting issue. I hadn't expected this to be a big deal because usually there is already a natural place in a coroutine that yields (e.g. whatever call used to come up with the next buffer of data to be written to the stream). This assumption fails in two cases: when writing a trivial test server that just produces an infinite stream of bytes; or when the source of the data is a very large disk file. The trap that both fall into is that when drain short-circuits the coroutine doesn't yield to the event loop at all, and the event loop needs to be invoked for the error to be set. But I am reluctant to make drain() yield on every call. I wonder if the right approach mightn't be for drain() to yield if the write buffer is fuller than the specified buffer limit. This is accessible via methods on the transport, which drain() can access.
This issue reminds me of issue #248 where I proposed to raise an error in write() is the transport is closing.
@Haypo: here's a hack that would cause drain() to yield when the transport is closing. Obviously this would be better if the transport had a closing property.
diff --git a/asyncio/streams.py b/asyncio/streams.py
index 6484c43..a91d992 100644
--- a/asyncio/streams.py
+++ b/asyncio/streams.py
@@ -301,6 +301,9 @@ def drain(self):
exc = self._reader.exception()
if exc is not None:
raise exc
+ if self._transport is not None:
+ if self._transport._closing:
+ yield
yield from self._protocol._drain_helper()
@gvanrossum I'm not an expert in that part of code. Could you please add a comment explaining why we need a yield there? I presume it's to let the loop process some closing coroutines? Is one yield enough?
I'll add a comment. The semantics of bare yield are explained by this
comment in tasks.py:
Bare yield relinquishes control for one event loop iteration
One yield is enough even if many steps were to be needed, because this is
only an issue if the app is looping, calling write() and drain() without
ever yielding anything else.
On Mon, Oct 19, 2015 at 9:39 AM, Yury Selivanov notifications@github.com
wrote:
@gvanrossum https://github.com/gvanrossum I'm not an expert in that
part of code. Could you please add a comment explaining why we need a
yield there? I presume it's to let the loop process the some closing
coroutines? Is one yield enough?—
Reply to this email directly or view it on GitHub
#263 (comment).
--Guido van Rossum (python.org/~guido)
Added the comment (use amend so it still looks like one commit).
Alright, with a comment the patch should be OK.
BTW, should we make 'transport._closing' a public API? IIRC @asvetlov requested that...
Fixed by #280. Also fixed upstream.