Skip to content

Comments

[3.6] bpo-26153: fix abort in _PyErr_WarnUnawaitedCoroutine during shutdown (GH-5337)#6536

Merged
Yhg1s merged 1 commit intopython:3.6from
Yhg1s:backport-dba976b-3.6
May 31, 2018
Merged

[3.6] bpo-26153: fix abort in _PyErr_WarnUnawaitedCoroutine during shutdown (GH-5337)#6536
Yhg1s merged 1 commit intopython:3.6from
Yhg1s:backport-dba976b-3.6

Conversation

@Yhg1s
Copy link
Member

@Yhg1s Yhg1s commented Apr 19, 2018

When an unawaited coroutine is collected very late in shutdown --
like, during the final GC at the end of PyImport_Cleanup -- then it
was triggering an interpreter abort, because we'd try to look up the
"warnings" module and not only was it missing (we were prepared for
that), but the entire module system was missing (which we were not
prepared for).

I've tried to fix this at the source, by making the utility function
get_warnings_attr robust against this in general. Note that it already
has the convention that it can return NULL without setting an error,
which is how it signals that the attribute it was asked to fetch is
missing, and that all callers already check for NULL returns.

There's a similar check for being late in shutdown at the top of
warn_explicit, which might be unnecessary after this fix, but I'm not
sure so I'm going to leave it..
(cherry picked from commit dba976b)

Co-authored-by: Nathaniel J. Smith njs@pobox.com

https://bugs.python.org/issue32591

…utdown (pythonGH-5337)

When an unawaited coroutine is collected very late in shutdown --
like, during the final GC at the end of PyImport_Cleanup -- then it
was triggering an interpreter abort, because we'd try to look up the
"warnings" module and not only was it missing (we were prepared for
that), but the entire module system was missing (which we were not
prepared for).

I've tried to fix this at the source, by making the utility function
get_warnings_attr robust against this in general. Note that it already
has the convention that it can return NULL without setting an error,
which is how it signals that the attribute it was asked to fetch is
missing, and that all callers already check for NULL returns.

There's a similar check for being late in shutdown at the top of
warn_explicit, which might be unnecessary after this fix, but I'm not
sure so I'm going to leave it..
(cherry picked from commit dba976b)

Co-authored-by: Nathaniel J. Smith <njs@pobox.com>
@Yhg1s Yhg1s changed the title [3.6] bpo-32591: fix abort in _PyErr_WarnUnawaitedCoroutine during shutdown (GH-5337) [3.6] bpo-26153: fix abort in _PyErr_WarnUnawaitedCoroutine during shutdown (GH-5337) Apr 19, 2018
@Yhg1s
Copy link
Member Author

Yhg1s commented Apr 19, 2018

the original commit message doesn't make much sense (the problem exists in 3.6, but the trigger in coroutine cleanup doesn't) -- but I've left it in place because it's a backport. I've pointed to the right issue for the non-coroutine crash, though.

@asvetlov asvetlov requested a review from 1st1 April 26, 2018 10:42
@1st1 1st1 requested a review from njsmith May 18, 2018 19:57
@1st1
Copy link
Member

1st1 commented May 18, 2018

@njsmith Nathaniel, can you take a look into this?

@njsmith
Copy link
Contributor

njsmith commented May 19, 2018

Looks fine to me.

I'd hit merge, but I'm not sure what the commit message is supposed to look like, so I'll leave that for someone else (maybe @Yhg1s?)

@1st1
Copy link
Member

1st1 commented May 19, 2018

I'd hit merge, but I'm not sure what the commit message is supposed to look like, so I'll leave that for someone else (maybe @Yhg1s?)

Hm, this actually looks like your commit, Nathaniel, backported to 3.6. So feel free to hit the button :)

@njsmith
Copy link
Contributor

njsmith commented May 19, 2018

It's my commit, but fixing an entirely different issue than the one described in my message, plus there's the extra backport metadata, and then there's the double-GH-links that I forget how you're supposed to handle. So yeah, I'm not confident about how best to do it.

@Yhg1s
Copy link
Member Author

Yhg1s commented May 31, 2018

I'm just going to merge it as-is then, to get this in 3.6.6. The commit message may be a little misleading but I don't think that's a big deal.

@Yhg1s Yhg1s merged commit 500a419 into python:3.6 May 31, 2018
@bedevere-bot
Copy link

@Yhg1s: Please replace # with GH- in the commit message next time. 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.

5 participants