gh-113753: Clear finalized bit when allocating PyAsyncGenASend from free-list #113754

colesbury · 2024-01-05T21:21:50Z

The PyAsyncGenASend may have their "finalized" bit set from previous use.

Issue: PyAsyncGenASend objects allocated from freelists may not have their finalizers called #113753

…from free-list

colesbury · 2024-01-05T21:24:23Z

@gvanrossum - this leads to a warning "RuntimeWarning: coroutine method 'asend' of 'AsyncGenTest.test_async_gen_exception_12..gen' was never awaited" in test_async_gen_exception_12. I can't figure out how to avoid or suppress the warning in this test case. Do you know how to deal with it?

cpython/Lib/test/test_asyncgen.py

Lines 380 to 391 in 99854ce

    
           def test_async_gen_exception_12(self): 
        
               async def gen(): 
        
                   await anext(me) 
        
                   yield 123 
        
               me = gen() 
        
               ai = me.__aiter__() 
        
               an = ai.__anext__() 
        
               with self.assertRaisesRegex(RuntimeError, 
        
                       r'anext\(\): asynchronous generator is already running'): 
        
                   an.__next__()

gvanrossum · 2024-01-05T21:44:21Z

Include/internal/pycore_gc.h

+static inline void _PyGC_CLEAR_FINALIZED(PyObject *op) {
+    PyGC_Head *gc = _Py_AS_GC(op);
+    gc->_gc_prev &= ~_PyGC_PREV_MASK_FINALIZED;
+}



This is expedient, but why not follow the patter used by FINALIZED and SET_FINALIZED that have a helper that skips the _Py_AS_GC() call?

I think those functions (_PyGCHead_SET_FINALIZED and _PyGCHead_FINALIZED) are likely to be removed in other PRs as I work on making the GC thread-safe in free-threaded builds. It's easier to support both builds when the functions take PyObject* instead of PyGC_Head* because the free-threaded builds will store the GC flags on the PyObject* itself (and eventually may not even have a PyGC_Head prefix.

I'm also OK with following the existing pattern for now and dealing with any potential removal later if you prefer.

Mostly I'm stuck on the warning generated in test_async_gen_exception_12. I don't know how to deal with it. Maybe @kumaraditya303 has suggestions?

gvanrossum · 2024-01-05T21:56:06Z

Cool. Undraft it and I'll merge it.

gvanrossum · 2024-01-05T21:56:45Z

Whoops didn't see the last msg. Will look at it later.

gvanrossum · 2024-01-06T01:22:27Z

I am totally baffled by this as well. That test always seems to give that warning:

test_async_gen_exception_12 (test.test_asyncgen.AsyncGenTest.test_async_gen_exception_12) ... /Users/guido/cpython/Lib/test/test_asyncgen.py:382: RuntimeWarning: coroutine method 'asend' of 'AsyncGenTest.test_async_gen_exception_12.<locals>.gen' was never awaited
  await anext(me)
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
ok

But I have no idea why in the CI for your PR it complains about a partial tracemalloc module -- it doesn't do that when I test it locally.

The only other clue I have is that if I run it locally and add --fast-ci to the test module's argument list it doesn't print that warning. The logic that leads to this result however eludes me. :-(

gvanrossum · 2024-01-06T16:38:32Z

Interesting, a similar error occurs in an unrelated PR (gh-11166), which also touches async generators. It's a different test, but the same phenomenon (AttributeError: partially initialized module 'tracemalloc' from '/home/runner/work/cpython/cpython-ro-srcdir/Lib/tracemalloc.py' has no attribute 'is_tracing' (most likely due to a circular import)). I wonder if the tracemalloc thing is unrelated and turns various warnings into errors.

gvanrossum · 2024-01-10T01:23:13Z

Interesting, a similar error occurs in an unrelated PR (gh-11166), which also touches async generators. It's a different test, but the same phenomenon (AttributeError: partially initialized module 'tracemalloc' from '/home/runner/work/cpython/cpython-ro-srcdir/Lib/tracemalloc.py' has no attribute 'is_tracing' (most likely due to a circular import)). I wonder if the tracemalloc thing is unrelated and turns various warnings into errors.

That problem is now fixed by GH-113813. I'll update the branch and maybe the tests will pass.

gvanrossum · 2024-01-10T02:39:53Z

Hah! This time all checks are passing.

colesbury · 2024-01-10T14:36:30Z

Thanks Guido!

Objects/genobject.c

kumaraditya303 · 2024-01-10T15:29:40Z

Objects/genobject.c

@@ -1951,6 +1952,7 @@ async_gen_wrapped_val_dealloc(_PyAsyncGenWrappedValue *o)
 #endif
    if (state->value_numfree < _PyAsyncGen_MAXFREELIST) {
        assert(_PyAsyncGenWrappedValue_CheckExact(o));
+        _PyGC_CLEAR_FINALIZED((PyObject *)o);


Oh clicked wrong button, you need to call this in async_gen_asend_dealloc on asend_freelist

Oops. I fixed the location of _PyGC_CLEAR_FINALIZED and updated test_asend in test_asyncgen.py so that it would catch this.

gvanrossum

I guess that test will fail if MAXFREELIST is zero. Fine.

I'll merge now.

pythongh-113753: Clear finalized bit when allocating PyAsyncGenASend …

7ae71e9

…from free-list

colesbury added the 3.13 new features, bugs and security fixes label Jan 5, 2024

colesbury requested a review from gvanrossum January 5, 2024 21:21

bedevere-app bot mentioned this pull request Jan 5, 2024

PyAsyncGenASend objects allocated from freelists may not have their finalizers called #113753

Closed

Add NEWS

fba7b02

gvanrossum approved these changes Jan 5, 2024

View reviewed changes

bedevere-app bot added the awaiting merge label Jan 5, 2024

gvanrossum mentioned this pull request Jan 6, 2024

AttributeError for tracemalloc.is_tracing #113781

Closed

Merge branch 'main' into pythongh-113753-asend

3856b92

colesbury marked this pull request as ready for review January 10, 2024 14:36

colesbury requested a review from markshannon as a code owner January 10, 2024 14:36

bedevere-app bot added awaiting review and removed awaiting merge labels Jan 10, 2024

kumaraditya303 reviewed Jan 10, 2024

View reviewed changes

Objects/genobject.c Outdated Show resolved Hide resolved

Move _PyGC_CLEAR_FINALIZED call

fe24eec

kumaraditya303 approved these changes Jan 10, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Jan 10, 2024

kumaraditya303 reviewed Jan 10, 2024

View reviewed changes

Fix location of _PyGC_CLEAR_FINALIZED call and add test

614ac7b

gvanrossum approved these changes Jan 10, 2024

View reviewed changes

gvanrossum merged commit 73ae202 into python:main Jan 10, 2024
32 checks passed

bedevere-app bot removed the awaiting merge label Jan 10, 2024

colesbury deleted the gh-113753-asend branch January 10, 2024 18:31

Dec	JAN	Feb
	12
2023	2024	2025

gh-113753: Clear finalized bit when allocating PyAsyncGenASend from free-list #113754

gh-113753: Clear finalized bit when allocating PyAsyncGenASend from free-list #113754

colesbury commented Jan 5, 2024 •

edited by bedevere-app bot

colesbury commented Jan 5, 2024

gvanrossum Jan 5, 2024

colesbury Jan 5, 2024

colesbury Jan 5, 2024

gvanrossum commented Jan 5, 2024

gvanrossum commented Jan 5, 2024

gvanrossum commented Jan 6, 2024

gvanrossum commented Jan 6, 2024

gvanrossum commented Jan 10, 2024

gvanrossum commented Jan 10, 2024

colesbury commented Jan 10, 2024

kumaraditya303 Jan 10, 2024

colesbury Jan 10, 2024

gvanrossum left a comment

gh-113753: Clear finalized bit when allocating PyAsyncGenASend from free-list #113754

gh-113753: Clear finalized bit when allocating PyAsyncGenASend from free-list #113754

Conversation

colesbury commented Jan 5, 2024 • edited by bedevere-app bot

colesbury commented Jan 5, 2024

gvanrossum Jan 5, 2024

Choose a reason for hiding this comment

colesbury Jan 5, 2024

Choose a reason for hiding this comment

colesbury Jan 5, 2024

Choose a reason for hiding this comment

gvanrossum commented Jan 5, 2024

gvanrossum commented Jan 5, 2024

gvanrossum commented Jan 6, 2024

gvanrossum commented Jan 6, 2024

gvanrossum commented Jan 10, 2024

gvanrossum commented Jan 10, 2024

colesbury commented Jan 10, 2024

kumaraditya303 Jan 10, 2024

Choose a reason for hiding this comment

colesbury Jan 10, 2024

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

colesbury commented Jan 5, 2024 •

edited by bedevere-app bot