Skip to content

bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). #12360

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

Merged

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Mar 15, 2019

This is effectively an un-revert of #11617 and #12024 (reverted in #12159). Portions of those were merged in other PRs (with lower risk) and this represents the remainder. Note that I found 3 different bugs in the original PRs and have fixed them here.

@vstinner, would you mind running this against that VM you set up, to see if it causes the original problem? I expect it won't.

https://bugs.python.org/issue33608

@ericsnowcurrently
Copy link
Member Author

@vstinner, actually let me fix the deadlock in test_capi first. I'll let you know when I'm ready. :)

@vstinner vstinner requested a review from pitrou March 20, 2019 02:38
@vstinner
Copy link
Member

Note for myself: the first time that this change has been merged, it caused issues at Python finalization, causing buildbot failures. https://bugs.python.org/issue33608#msg337116

@ericsnowcurrently
Copy link
Member Author

FYI, I've fixed the problem. :) @vstinner, could you try this branch against that VM you set up?

@ericsnowcurrently
Copy link
Member Author

I plan on merging this on Friday (in 2 days). If we still see the same buildbot failures (or @vstinner still sees the problem in his VM), we can go from there. If a revert (again) is necessary, it won't be nearly as disruptive since this PR is much more focused. However, I don't expect any problems at this point.

Since the revert I've re-merged most of the original changes (as noted above, in the PR summary), with no apparent negative impact. I've also landed several fixes (e.g. to finalization, to locks after fork), found while working on the un-revert, which likely prevent the buildbot failures for which the revert happened. Furthermore, this PR includes several fixes (as noted above) that likewise probably help with those revert-motivating buildbot failures. Consequently I'm confident that the underlying issue has been resolved.

@ericsnowcurrently ericsnowcurrently merged commit f13c5c8 into python:master Apr 12, 2019
@vstinner
Copy link
Member

I have a bad news for you Eric: I'm able again to reproduce the crash at commit f13c5c8: https://bugs.python.org/issue33608#msg340069

@ericsnowcurrently
Copy link
Member Author

That's too bad. I'll revert then.

@ericsnowcurrently
Copy link
Member Author

Thanks for checking that.

@ericsnowcurrently
Copy link
Member Author

@vstinner, it isn't failing without this branch, right?

ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Apr 12, 2019
@vstinner
Copy link
Member

@vstinner, it isn't failing without this branch, right?

I ran my test using 2 terminals for 8 minutes: I failed to reproduce the crash, whereas with your change I got a crash way faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants