Skip to content
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

bpo-42972: Fix GC assertion error in _winapi by untracking Overlapped earlier #26429

Merged
merged 1 commit into from
May 28, 2021

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-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
Member Author

@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
Member Author

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.

@vstinner vstinner merged commit 490b638 into python:main May 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-Spinner Fidget-Spinner deleted the fix-httplib branch May 28, 2021 16:34
@Fidget-Spinner
Copy link
Member Author

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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants