This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Set closing variable in asyncore at close
Type: Stage: resolved
Components: Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: Nir Soffer, giampaolo.rodola, serhiy.storchaka, vstinner, walkhour
Priority: normal Keywords:

Created on 2017-07-21 16:06 by walkhour, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 2804 closed walkhour, 2017-07-21 16:07
Messages (12)
msg298815 - (view) Author: Nir Soffer (Nir Soffer) Date: 2017-07-21 18:26
This is an old issue with asyncore - asyncore has a "closing" attribute,
but it was never used. Every user has to implement the closing once
logic in dispatcher subclasses.

Here is a typical fixes in user code:
- https://github.com/oVirt/vdsm/blob/master/lib/vdsm/storage/asyncevent.py#L497
- https://github.com/oVirt/vdsm/blob/master/lib/vdsm/storage/asyncevent.py#L540

Fixing closing attribute will allow fixing the races during
asyncore.poll() and asyncore.poll2(), see
https://github.com/python/cpython/pull/2764
msg299026 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-07-24 22:39
Proposed change looks more like an enhancement like a bug fix.

I consider that features are not welcome anyone in the asyncore module, since it's now deprecated, replaced with selectors and asyncio.

I would prefer to reject this issue.

Giampaolo, Serhiy, Guido: what do you think?


> Every user has to implement the closing once logic in dispatcher subclasses.

Even if it's painful, I consider that it's an acceptable compromise.
msg299028 - (view) Author: Nir Soffer (Nir Soffer) Date: 2017-07-24 22:54
I agree that this change alone is may not be important enough to fix in asyncore today - but this enables easy detection of closed sockets needed for https://github.com/python/cpython/pull/2764 or the alternative https://github.com/python/cpython/pull/2854. Unless you suggest a better way to detect closed dispatchers.

Introducing new failures modes in asyncore is extremely risky, error handling in asyncore is the weakest point.
msg299032 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-07-24 23:31
"Fixing closing attribute will allow fixing the races during asyncore.poll() and asyncore.poll2(), see https://github.com/python/cpython/pull/2764"

Aha, *slowly* I understand that asyncore is vulnerable to multiple race conditions, and it seems lile we require to set a new closing attribute to fix it.
msg299134 - (view) Author: Nir Soffer (Nir Soffer) Date: 2017-07-25 19:14
The "new" closing attribute is old as asyncore, it was just unused :-)
msg299145 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2017-07-25 21:25
I welcome this change as it avoids tricks like this one: 
https://github.com/giampaolo/pyftpdlib/blob/1268bb185cd63c657d78bc33309041628e62360a/pyftpdlib/handlers.py#L537
Any app using asyncore for anything other than an echo server eventually ends up doing the same.
msg299147 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2017-07-25 21:33
On the other hand, due to the poor asyncore API, I think it's safer if we land this in 3.7 only. "closing" attribute was never documented so if there's code out there setting it to True that'll crash their app pretty quickly as sockets won't close.
msg299151 - (view) Author: Nir Soffer (Nir Soffer) Date: 2017-07-25 22:00
Giampaolo, people using only 3.7 should probably use asyncio. Fixing
asyncore is more important to those that can use only 2.7 (e.g.Centos/RHEL)
or have to support both python 3 and 2.

Do you think using _closed is safer for backport? This can also clash with
existing code.
msg299154 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-07-25 22:14
"Giampaolo, people using only 3.7 should probably use asyncio. Fixing
asyncore is more important to those that can use only 2.7 (e.g.Centos/RHEL) or have to support both python 3 and 2."

IMHO starting to use closing in asyncore *is* a backward incompatible change. We don't know how applications use this existing attribute. Maybe it's not set to a boolean. Maybe it uses a different policy.

I would be ok to start using closing in Python 3.7, but I'm not confortable with backporting such change.

I would be if would be add a new private attribute, as the proposed "_closed" name.

Maybe nobody uses closing. Maybe people using closing have a similar usage. The thing is that we don't know, and according to what you wrote Nir, closing *is* used.

What I dislike in asyncore is that subclassing is not prohibed, it seems wanted by design. Ok, but what if a subclass overrides completely a method and doesn't set "closing/_closed" anymore? That's another reason why I dislike the idea of making any change in asyncore *especially* in Python 2.7 which is now considered as "very stable".
msg299165 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-07-25 23:24
It seems like the closing attribute is not *required* to fix bpo-30931, asyncore race condition:
http://bugs.python.org/issue30931#msg299162

Another approach doesn't rely on close() "implementation", but more on close() "expected behaviour" (remove the dispatcher file descriptor from map). IMHO this approach would reduce the risk of backward incompatibility.

--

I'm still in favor of rejecting this feature request, *but* I would prefer to first wait until we agree how to fix bpo-30931 race condition.
msg299171 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2017-07-26 00:07
I would probably feel safer to use "__closed" for all python versions and adopt the same naming convention for any attribute we may want to add in the future. Kinda weird, but asyncore is probably the only case of deprecated module with bad API design which makes it risky to make any change to the class namespace. As such I consider using the ugly "__" prefix justifiable.
msg324693 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-06 14:27
I reject the issue. It doesn't seem possible to set the closing attribute without breaking backward compatibility. See discussion:
https://github.com/python/cpython/pull/2804
History
Date User Action Args
2022-04-11 14:58:49adminsetgithub: 75168
2018-09-06 14:27:05vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg324693

stage: resolved
2017-07-26 00:07:14giampaolo.rodolasetmessages: + msg299171
2017-07-25 23:24:08vstinnersetmessages: + msg299165
2017-07-25 22:14:30vstinnersetmessages: + msg299154
2017-07-25 22:08:22gvanrossumsetnosy: - gvanrossum
2017-07-25 22:00:16Nir Soffersetmessages: + msg299151
2017-07-25 21:33:24giampaolo.rodolasetmessages: + msg299147
2017-07-25 21:25:55giampaolo.rodolasetmessages: + msg299145
2017-07-25 19:14:14Nir Soffersetmessages: + msg299134
2017-07-24 23:31:27vstinnersetmessages: + msg299032
2017-07-24 22:54:17Nir Soffersetmessages: + msg299028
2017-07-24 22:39:12vstinnersetnosy: + gvanrossum, giampaolo.rodola, serhiy.storchaka
messages: + msg299026
2017-07-21 18:26:51Nir Soffersetnosy: + vstinner
2017-07-21 18:26:29Nir Soffersetnosy: + Nir Soffer

messages: + msg298815
versions: + Python 2.7, Python 3.5, Python 3.6, Python 3.7
2017-07-21 16:07:51walkhoursetpull_requests: + pull_request2854
2017-07-21 16:06:42walkhourcreate