classification
Title: asyncore: allow handling of half closed connections
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: giampaolo.rodola Nosy List: Isaac Boukris, asvetlov, eamanu, giampaolo.rodola, stutzbach
Priority: normal Keywords: patch, patch

Created on 2019-02-06 15:51 by Isaac Boukris, last changed 2019-05-02 04:16 by josiahcarlson.

Pull Requests
URL Status Linked Edit
PR 11770 open python-dev, 2019-02-06 15:56
PR 11770 open python-dev, 2019-02-06 15:57
Messages (12)
msg334944 - (view) Author: Isaac Boukris (Isaac Boukris) * Date: 2019-02-06 15:51
When recv() return 0 we may still have data to send. Add a handler for this case, which may happen with some protocols, notably http1.0 ver.

Also, do not call recv with a buffer size of zero to avoid ambiguous return value (see recv man page).
msg334960 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2019-02-06 17:28
Assigning this to me but am not sure 1) when I'll be able to look at this 2) whether it's worth it as asyncore is deprecated in favor of asyncio.
msg334961 - (view) Author: Emmanuel Arias (eamanu) * Date: 2019-02-06 17:35
Hi!

> Assigning this to me but am not sure 1) when I'll be able to look at this 2) whether it's worth it as asyncore is deprecated in favor of asyncio.

Yes, asyncore is deprecated since 3.6.
msg334964 - (view) Author: Isaac Boukris (Isaac Boukris) * Date: 2019-02-06 17:49
Fair enough. I'll sign the CLA meanwhile you consider it.

In my opinion it may still be useful in addressing issues in existing projects written using asyncore (and maybe for python2 as well).

Thanks!
msg334968 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-02-06 17:59
My personal opinion is: we should accept bug fixes for asyncore but stop adding new features to the module.
asyncio supersedes asyncore in all aspects.
msg334976 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2019-02-06 19:58
> When recv() return 0 we may still have data to send.

It seems recv() returning b"" is an alias for "connection lost". E.g. in Twisted:
https://github.com/twisted/twisted/blob/06c891502be9f6389451fcc959cad5485f55d653/src/twisted/internet/tcp.py#L227-L248
msg334978 - (view) Author: Isaac Boukris (Isaac Boukris) * Date: 2019-02-06 20:13
> It seems recv() returning b"" is an alias for "connection lost". E.g. in Twisted:

To my understanding, technically the connection is not fully closed, it is just shut-down for reading but we can still perform write operations on it (that is, the client may be still waiting for the response). I can reproduce it with an http-1.0 client, I'll try to put up a test to demonstrate it more clearly.

From recv() man page:
When a stream socket peer has performed an orderly shutdown, the return value will be 0 (the traditional "end-of-file" return).
msg334985 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-02-06 21:16
Technically the change seems correct, we have the same logic for asyncio half-closed streams.

But I want to raise the flag again: why we are adding new functionality to the *deprecated* module? It violates our on deprecation policy, isn't it?
msg334986 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2019-02-06 21:27
I agree. The problem I have with this is that it introduces a new method (handle_eof), which ends up in the "new functionality" bucket (even though it's not backward incompatible per-se, as it defaults on calling handle_close() anyway, but still it is technically a new API). Point with asyncore/chat is that every time you try to fix them you end up messing with the public API one way or another.

I know from experience (pyftpdlib) that all asyncore/chat users already subclass/overwrite the base classes quite massively, so if they really want to rely on this new functionality they probably already have implemented it themselves.
msg334990 - (view) Author: Isaac Boukris (Isaac Boukris) * Date: 2019-02-06 22:56
> But I want to raise the flag again: why we are adding new functionality to the *deprecated* module? It violates our on deprecation policy, isn't it?

I'm biased but I see this as more of a small and subtle fix for the current logic that incorrectly treats this as a closed connection, rather than a new feature.
In addition, it could serve a documentation hint for people troubleshooting edge cases in their code (especially people who are not familiar with these semantics).

> Point with asyncore/chat is that every time you try to fix them you end up messing with the public API one way or another.

I'd agree about the first commit (avoid calling recv with size zero), which may change the behavior for a poorly written application that tries to read a chunk of zero bytes, but the second commit is straight forward and I can't see how it could break anything.
msg335008 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-02-07 07:59
The change adds a new public method.
The method should be added to documentation also.
Documentation requires `.. versionadded:: 3.8` tag.
It doesn't look like *just a bugfix* but a feature.
msg335009 - (view) Author: Isaac Boukris (Isaac Boukris) * Date: 2019-02-07 08:25
if not data:
                # a closed connection is indicated by signaling
                # a read condition, and having recv() return 0.
                self.handle_close()
                return b''

This above is the current code. Do you agree that it makes a wrong assumption and therefore behave incorrectly? If so, how do you suggest fixing it without adding a new method?

Otherwise; maybe we can at least amend the comment in the code, and perhaps add a word or two to the doc.
History
Date User Action Args
2019-05-02 04:16:32josiahcarlsonsetkeywords: patch, patch
nosy: - josiahcarlson
2019-02-07 08:25:16Isaac Boukrissetmessages: + msg335009
2019-02-07 07:59:44asvetlovsetkeywords: patch, patch

messages: + msg335008
2019-02-06 22:56:33Isaac Boukrissetmessages: + msg334990
2019-02-06 21:27:11giampaolo.rodolasetkeywords: patch, patch

messages: + msg334986
2019-02-06 21:16:12asvetlovsetkeywords: patch, patch

messages: + msg334985
2019-02-06 20:13:22Isaac Boukrissetmessages: + msg334978
2019-02-06 19:58:13giampaolo.rodolasetkeywords: patch, patch

messages: + msg334976
2019-02-06 17:59:04asvetlovsetkeywords: patch, patch
nosy: + asvetlov
messages: + msg334968

2019-02-06 17:49:04Isaac Boukrissetmessages: + msg334964
2019-02-06 17:35:56eamanusetnosy: + eamanu
messages: + msg334961
2019-02-06 17:28:24giampaolo.rodolasetkeywords: patch, patch
assignee: giampaolo.rodola
messages: + msg334960
2019-02-06 16:36:51SilentGhostsetkeywords: patch, patch
nosy: + josiahcarlson, giampaolo.rodola, stutzbach

versions: + Python 3.8
2019-02-06 15:57:01python-devsetkeywords: + patch
stage: patch review
pull_requests: + pull_request11736
2019-02-06 15:56:59python-devsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11735
2019-02-06 15:51:04Isaac Boukriscreate