gh-93821: Fix bug that accepting a socket connection and ERROR_NETNAME_DELETED occurs causes serving socket to be closed #124032
Conversation
…ETED occurs, leads this into a closing of the serving socket This is done with the patches described by fercod in the GitHub thread python#93821. These patches were successfully tested by me and worked properly. The error propagates properly without closing the original socket and without causing the server to hang.
StanHatko
requested review from
1st1,
asvetlov,
gvanrossum,
kumaraditya303 and
willingc
as code owners
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
No need for checking winerror present as windows_events.py only imported on Windows (with check near top). Make comment more clear as to purpose of this patch.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Second edit was not necessary since windows_events.py is always windows-only, line 5: if sys.platform != 'win32': # pragma: no cover
raise ImportError('win32 only')I'll revert that change to the patch. |
|
It seems the patch works, I'll just need to do a test on a Windows computer with the dev Python from the main branch to be sure. Once that's done I'll add the blurb NEWS entry and will mark it for review so it can be merged into Python. |
|
I did some testing with the new version and it was working properly for me, it fixed the bug #93821. Is the blurb now the only thing missing, or would you like any other changes made before accepting this pull request? |
Fixes issue #93821, using patch suggested by fercod in that thread. I tested that patch and found it worked properly on a Windows system, given the MWE at the top of the thread. I made a small change as I think accessing winerror within the exception handler could cause another exception on Linux and other systems without the winerror member of OSError.
I still want to do a bit more testing with Linux and Windows systems. It passed various default tests I ran (on a Linux computer) but we'd need to test with the relevant exceptions actually occurring in those spots to make sure it fully works everywhere. I hope to do that over the next few days.