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
asyncore.dispatcher.recv doesn't handle EAGAIN / EWOULDBLOCK #60337
Comments
I recently had lots of the following exception: Error 11 is EAGAIN or EWOULDBLOCK, so asyncore/asynchat tries to read from a nonblocking socket which has no data available. Since this is a temporary error the socket shouldn't be closed. The bug can be fixed by changing asyncore.dispatcher.recv to While looking at the source I also saw that asyncore.dispatcher.send and .connect check against EWOULDBLOCK but not against EAGAIN. Since both constants may have different values and POSIX allows to return either of them these functions should check against both error constants. |
I confirm I can reproduce this issue also in pyftpdlib: |
Patch is in attachment. |
Why should asynchat.handle_read care about closed sockets if asyncore.recv does that already? Changed the patch accordingly. |
recv() returning an empty string has always been an alias for "connection lost" though, that is why it cannot be used and I was proposing returning a new type in Python 3.4. Point is we're paying a bad design decision: asyncore shouldn't have asked the user to call recv() directly in the first place and call a data_received(chunk) callback method instead. Deciding what's best to do at this point without breaking existent code is not easy, that is why I think that on python <= 3.3 we should fix *asynchat* in order to take EAGAIN/EWOULDBLOCK into account and leave asyncore's recv() alone. As for Python 3.4 we can:
|
For the reson why read() must still check for EWOULDBLOCK even though
|
IMHO for all python versions, asynchat should be fixed and recv() left The attached patch does this on the default branch. |
Could we have a review of the latest path from Xavier please. Aside, msg172160 "add Windows project file for _sha3 module. I choose to build _sha3 as a sparat module as it's rather large (190k for AMD64).", presumably Christian mistyped an issue number? |
EWOULDBLOCK.patch: asyncio ignores BlockingIOError on sock.recv(), "except BlockingIOError:" is more portable and future proof than "_RETRY = frozenset((EWOULDBLOCK, EAGAIN))". Except of that, EWOULDBLOCK.patch change looks correct. |
Modifying recv() to return None doesn't look correct. I read it as: "you should always use recv() output, except if the result is None: in this case, do nothing". In Python, we use exceptions for that. BUT in fact, sock.recv() already raises an exception, so asyncore should not convert the exception to a magic value (None). Modifying the behaviour of recv() in asyncore breaks the backward compatibility. Returning None makes it harder to write asyncore code working on Python 3.4 and 3.5 (if the change is done in Python 3.5). I prefer EWOULDBLOCK.patch approach: document the issue in asyncore documentation and handle BlockingIOError in asynchat. |
See also the issue bpo-15982 which is the exactly the same on Windows. (By the way, the asyncore module has been marked as deprecated in Python 3.4 in favor of asyncio, and this issue is already solved in asyncio.) |
New changeset b7f144d14798 by Victor Stinner in branch '3.4': New changeset aa150c7a5d24 by Victor Stinner in branch 'default': |
New changeset d422062d7d36 by Victor Stinner in branch '2.7': |
I modified EWOULDBLOCK.patch to use BlockingIOError on Python 3 and I added a unit test. I also added EALREADY and EINPROGRESS which are used by the BlockingIOError in Python 3, just in case. Thanks Xavier for your patch, sorry for the delay. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: