Skip to content

Conversation

@Fidget-Spinner
Copy link
Member

@Fidget-SpinnerFidget-Spinner commented May 28, 2021

See #26381 (comment). This untracks a possible 'bad' object earlier so the GC stops having weird race conditions. Fixes a gc assertion error in debug mode in test_httplib.

python_d.exe -m test test_httplib -m test_local_bad_hostname ...\cpython\Modules\gcmodule.c:446: update_refs: Assertion "gc_get_refs(gc) != 0" failed Enable tracemalloc to get the memory block allocation traceback object address : 000002DADE2A99D0 object refcount : 0 object type : 000002DADDF72F60 object type name: _winapi.Overlapped object repr : <refcnt 0 at 000002DADE2A99D0> Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed Python runtime state: initialized 

https://bugs.python.org/issue42972

@Fidget-Spinner
Copy link
MemberAuthor

@vstinner seems like your gut feeling about UnTrack was right

@vstinner
Copy link
Member

test_httplib doesn't use asyncio, so it's unrelated. The test_httplib crash is: https://bugs.python.org/issue44252

python_d.exe -m test test_httplib -m test_local_bad_hostname ...\cpython\Modules\gcmodule.c:446: update_refs: Assertion "gc_get_refs(gc) != 0" failed Enable tracemalloc to get the memory block allocation traceback 

@vstinner
Copy link
Member

vstinner commented May 28, 2021

If you want to modify overlapped_dealloc(), I suggest to also remove the compatibility code for Windows XP.

 if (self->pending){if (check_CancelIoEx() && Py_CancelIoEx(self->handle, &self->overlapped) && GetOverlappedResult(self->handle, &self->overlapped, &bytes, TRUE)){/* The operation is no longer pending -- nothing to do. */ } else if (_Py_IsFinalizing()){/* The operation is still pending -- give a warning. This will probably only happen on Windows XP. */ PyErr_SetString(PyExc_RuntimeError, "I/O operations still in flight while destroying " "Overlapped object, the process may crash"); PyErr_WriteUnraisable(NULL)} else{/* The operation is still pending, but the process is probably about to exit, so we need not worry too much about memory leaks. Leaking self prevents a potential crash. This can happen when a daemon thread is cleaned up at exit -- see #19565. We only expect to get here on Windows XP. */ CloseHandle(self->overlapped.hEvent); SetLastError(err); return} } 

check_CancelIoEx() can be removed and CancelIoEx() can be used directly. It's available since Windows Vista and Python 3.11 requires Windows Vista or newer:
https://docs.microsoft.com/en-us/windows/win32/fileio/cancelioex-func

See also bpo-32592 "Drop support of Windows Vista and Windows 7".

Please open a separated PR for that.

I don't think that it's worth it to only move the PyObject_GC_UnTrack() call.

@Fidget-Spinner
Copy link
MemberAuthor

Fidget-Spinner commented May 28, 2021

test_httplib doesn't use asyncio, so it's unrelated. The test_httplib crash is: https://bugs.python.org/issue44252

python_d.exe -m test test_httplib -m test_local_bad_hostname ...\cpython\Modules\gcmodule.c:446: update_refs: Assertion "gc_get_refs(gc) != 0" failed Enable tracemalloc to get the memory block allocation traceback 

Sorry, I was referring to another crash. These 2 build bots crashed when running in python debug mode due to my previous commit:
https://buildbot.python.org/all/#/builders/146/builds/294/steps/4/logs/stdio
https://buildbot.python.org/all/#/builders/596/builds/323/steps/4/logs/stdio

Both due to assertion errors in the GC. So I wanted to fix those first. The Windows support ones can come another time.

@vstinnervstinner merged commit 490b638 into python:mainMay 28, 2021
@vstinner
Copy link
Member

Sorry, I was referring to another crash. These 2 build bots crashed when running in python debug mode due to my previous commit:

Ah ok, I merged your PR.

@vstinner
Copy link
Member

Can you please create a 3.10 backport which contains your two commits? Use git cherry-pick -x.

@vstinner
Copy link
Member

I closed #26431 which only contained the first fix.

@Fidget-SpinnerFidget-Spinner deleted the fix-httplib branch May 28, 2021 16:34
@Fidget-Spinner
Copy link
MemberAuthor

Can you please create a 3.10 backport which contains your two commits? Use git cherry-pick -x.

Yep, #26430 has both fixes. Thanks!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Fidget-Spinner@vstinner@the-knights-who-say-ni@bedevere-bot