bpo-31061: fix crash in asyncio speedup module #2966

thehesiod · 2017-08-01T01:24:12Z

https://bugs.python.org/issue31061

follows what was done in: https://bugs.python.org/issue26617
fallout in: http://bugs.python.org/issue31095

thehesiod · 2017-08-01T01:26:53Z

this is my first cpython PR, let me know what else needs to be done to get this into any other releases it's required in.

methane · 2017-08-01T04:16:07Z

Modules/_asynciomodule.c

    if (Future_CheckExact(fut)) {
        /* When fut is subclass of Future, finalizer is called from
         * subtype_dealloc.
         */
+         PyObject_GC_Track(self);


Is this really needed?

sorry, you're right. it's needed.

I'm just following what was done in https://bugs.python.org/issue26617. I think it's worth someone grepping through the whole codebase, I saw several other places that maybe needed this. It seems like every dealloc should have a PyObject_GC_UnTrack at the top?

Maybe, noddy4 example should do it at first :)

btw how do we get this into 3.6.3 as well?

It seems like every dealloc should have a PyObject_GC_UnTrack at the top?

When the type has Py_TPFLAGS_HAVE_GC.
lru_cache_dealloc in _functoolsmodule seems need it.

I filed an issue for that. http://bugs.python.org/issue31095

k, should I remove the LRU change I added here?

Yes, please. asyncio speedup module is introduced from Python 3.6.
It make easy to backport.

methane · 2017-08-01T04:21:11Z

Since _asinciomodule is compiled with python interpreter, it can use private macro APIs
(_PyObject_GC_TRACK and _PyObject_GC_UNTRACK).

methane · 2017-08-01T04:23:21Z

Oh, sorry, macro version API broke Windows build and you used public APIs to fix it.

methane · 2017-08-01T06:06:32Z

Modules/_asynciomodule.c

@@ -962,14 +962,18 @@ FutureObj_dealloc(PyObject *self)
 {
    FutureObj *fut = (FutureObj *)self;

+    PyObject_GC_UnTrack(self);
+
    if (Future_CheckExact(fut)) {


Please move PyObject_GC_UnTrack after this if block, and remove Track and Untrack in the block.

but that would mean this (34fae03) is wrong too then no?

because it first does an UnTrack: 34fae03#diff-c3cf251f16d5a03a9e7d4639f2d6f998L1113

With the same Track/Untrack pattern here:
34fae03#diff-c3cf251f16d5a03a9e7d4639f2d6f998R1129

current code:

UnTrack() Track() PyObject_CallFinalizerFromDealloc() UnTrack()

I proposed:

PyObject_CallFinalizerFromDealloc() UnTrack()

34fae03 does other things between UnTrack() and Track()
But this code does only Future_CheckExact() and it is safe.

k, I'm claiming ignorance since I don't know what these macros achieve :) I'd personally feel safer if we had a test case, do you know how to create a testcase to cause this to crash? Mine takes a day to reproduce in a production environment :(

CPython's circular reference GC uses doubly linked list to track object which can be member of circular reference.
PyObject_GC_UnTrack() removes object from the list, and PyObject_GC_Track() insert the object to the list.

Despite very undetarministic multithreading, I can cause SEGV quickly with this code:

import asyncio import gc gc.set_debug(gc.DEBUG_STATS) class Evil: def __del__(self): gc.collect() def main(): f = asyncio.Future() f.set_result(Evil()) for i in range(100): main()

nice! I'll add testcases and add your suggestions, thanks so much for all the help!

k, added FutureObj testcase, have a good test case for the TaskObj? Not quite sure how to trigger it.

methane · 2017-08-01T06:10:32Z

Could you add news entry?
https://devguide.python.org/committing/#what-s-new-and-news-entries

thehesiod · 2017-08-01T06:14:39Z

added news and reverted LRU change

thehesiod · 2017-08-01T09:25:04Z

should we put as many fixes as we can in this PR, or create another PR with as many as we can? I need the defaultdict one too...and maybe even others as who knows what third party libraries do. I feel like we've opened a pandoras box of crashers :)

methane · 2017-08-01T09:55:22Z

I already created #2974.
More pull request for updating document is welcome.

methane · 2017-08-01T10:00:50Z

Include/objimpl.h

+
+/*
+   See description in Doc/c-api/gcsupport.rst
+*/


Please remove it.
We can reach the document easily with searching API name.

1st1 · 2017-08-01T13:59:25Z

The latest revision LGTM. But can you use blurb to generate the NEWS entry?

thehesiod · 2017-08-01T16:34:32Z

@1st1 should I remove the one in NEWS? Also was hoping I could add a testcase for Task but couldn't figure out how to repro it, especially given Task inherits from Future so I don't want to trigger the Future's crash

1st1 · 2017-08-01T16:38:14Z

Misc/NEWS

@@ -10,6 +10,8 @@ What's New in Python 3.7.0 alpha 1?
 Core and Builtins
 -----------------

+- bpo-31061: Fixed a crash when using asyncio and threads.


Last nit: the change in Misc/NEWS needs to be reverted, we need only blurb file.

1st1 · 2017-08-01T16:39:07Z

@1st1 should I remove the one in NEWS?

Yes, please!

Also was hoping I could add a testcase for Task but couldn't figure out how to repro it, especially given Task inherits from Future so I don't want to trigger the Future's crash

I'm trying to come up with a test.

1st1 · 2017-08-01T16:47:41Z

The following test crashes unpatched CPython:

    import gc

    def test_task_del_collect(self):
        async def coro():
            return Evil()

        class Evil:
            def __del__(self):
                gc.collect()

        for i in range(100):
            task = self.new_task(self.loop, coro())
            self.loop.run_until_complete(task)

1st1 · 2017-08-01T16:50:19Z

The test also fails if we fix only Task, but not Future.

thehesiod · 2017-08-01T17:26:43Z

so I had something similar but it doesn't crash:

import gc
import asyncio

class Evil:
    def __del__(self):
        gc.collect()


async def run():
    return Evil()


loop = asyncio.get_event_loop()

for i in range(100):
    loop.run_until_complete(asyncio.Task(run()))
#    f = asyncio.Future()
#    f.set_result(Evil())

thehesiod · 2017-08-01T17:33:01Z

ah, figured it out, had to have multiple tasks pending in the run loop.

1st1 · 2017-08-01T18:08:35Z

Lib/test/test_asyncio/test_tasks.py

+        def run():
+            return Evil()
+
+        yield from asyncio.gather(*[asyncio.Task(run()) for _ in range(100)])


This test won't run. yield from makes test_task_del_collect() a generator and test suite doesn't know how to run them.

Please change to

self.loop.run_until_complete( asyncio.gather(*[asyncio.Task(run()) for _ in range(100)]))

thehesiod · 2017-08-01T19:01:25Z

ya realized as I was going to work, this should fix it

1st1

I have no further comments, LGTM. @methane do you approve this PR?

1st1 · 2017-08-01T20:33:58Z

Lib/test/test_asyncio/test_tasks.py

@@ -4,6 +4,7 @@
 import contextlib
 import functools
 import io
+import gc


Another nit: 'gc' import should be before 'functools' -- we sort imports alphabetically in CPython.

may be worth running a linter on CI to catch these

I agree, good idea. BTW help is always welcome by python/core-workflow. You're an awesome contributor, we'd love you to continue! :)

you're too kind :) Will keep that in mind if we run into any other cpython issues.

thehesiod · 2017-08-01T20:43:33Z

this set of PRs is a massive win for Python. It would be nice if a core dev could look and find a way to nip it at the bud to avoid these hacky calls, as shown they're highly error prone. Who knows how many third party modules are missing them too.

methane

LGTM

(cherry picked from commit de34cbe)

bedevere-bot · 2017-08-02T07:11:44Z

GH-2984 is a backport of this pull request to the 3.6 branch.

1st1 · 2017-08-02T12:44:49Z

Do we have C Task/Future in 3.5?

methane · 2017-08-02T12:47:27Z

No. asyncio speedup is introduced at 3.6

attempt to fix http://bugs.python.org/issue31061

336a6ce

the-knights-who-say-ni added the CLA signed label Aug 1, 2017

1st1 requested a review from methane August 1, 2017 01:25

1st1 self-assigned this Aug 1, 2017

thehesiod changed the title ~~attempt to fix http://bugs.python.org/issue31061~~ attempt to fix bpo-31061 Aug 1, 2017

thehesiod added 2 commits July 31, 2017 18:29

fix typo

4fd34a6

fix capitalization

97ccf31

methane reviewed Aug 1, 2017

View reviewed changes

thehesiod added 2 commits July 31, 2017 21:23

switch to private version

a67847e

undo :)

e126945

methane approved these changes Aug 1, 2017

View reviewed changes

add missing untrack

de3ee01

methane added the needs backport to 3.6 label Aug 1, 2017

methane requested changes Aug 1, 2017

View reviewed changes

thehesiod added 2 commits July 31, 2017 23:11

revert change

6a02050

add news item

503806f

simplify patch based on review

f900058

methane added the type-bug An unexpected behavior, bug, or error label Aug 1, 2017

methane reviewed Aug 1, 2017

View reviewed changes

methane changed the title ~~attempt to fix bpo-31061~~ bpo-31061: fix crash in asyncio speedup module Aug 1, 2017

thehesiod added 2 commits August 1, 2017 09:19

remove

483586b

add blurb

b4589ce

remove space

fa2b42b

1st1 reviewed Aug 1, 2017

View reviewed changes

revert

3ca521d

new task test case

b0fb8e7

1st1 reviewed Aug 1, 2017

View reviewed changes

fix unittest

f32e949

thehesiod added 2 commits August 1, 2017 12:05

set the loop

7bc0c2a

fix off by one paren error :)

55a2eb5

1st1 approved these changes Aug 1, 2017

View reviewed changes

1st1 reviewed Aug 1, 2017

View reviewed changes

fix ordering

3033b15

methane approved these changes Aug 2, 2017

View reviewed changes

methane merged commit de34cbe into python:master Aug 2, 2017

methane pushed a commit to methane/cpython that referenced this pull request Aug 2, 2017

[3.6] bpo-31061: fix crash in asyncio speedup module (pythonGH-2966)

bafb2c4

(cherry picked from commit de34cbe)

bedevere-bot removed the needs backport to 3.6 label Aug 2, 2017

thehesiod deleted the thehesiod/aio_crash_fix branch August 2, 2017 19:29

bpo-31061: fix crash in asyncio speedup module #2966

bpo-31061: fix crash in asyncio speedup module #2966

Conversation

thehesiod commented Aug 1, 2017 • edited Loading

thehesiod commented Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methane Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methane commented Aug 1, 2017

methane commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methane commented Aug 1, 2017

thehesiod commented Aug 1, 2017

thehesiod commented Aug 1, 2017

methane commented Aug 1, 2017

Choose a reason for hiding this comment

1st1 commented Aug 1, 2017

thehesiod commented Aug 1, 2017

Choose a reason for hiding this comment

1st1 commented Aug 1, 2017

1st1 commented Aug 1, 2017 • edited Loading

1st1 commented Aug 1, 2017

thehesiod commented Aug 1, 2017

thehesiod commented Aug 1, 2017

Choose a reason for hiding this comment

thehesiod commented Aug 1, 2017 • edited Loading

1st1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehesiod commented Aug 1, 2017

methane left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 2, 2017

1st1 commented Aug 2, 2017

methane commented Aug 2, 2017

thehesiod commented Aug 1, 2017 •

edited

Loading

thehesiod commented Aug 1, 2017 •

edited

Loading

methane Aug 1, 2017 •

edited

Loading

1st1 commented Aug 1, 2017 •

edited

Loading

thehesiod commented Aug 1, 2017 •

edited

Loading