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.

Sign up for GitHub

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

Jump to bottom

gh-94440: Fix issue of ProcessPoolExecutor shutdown hanging #94468

Merged
merged 8 commits into from
Mar 16, 2023
Merged

gh-94440: Fix issue of ProcessPoolExecutor shutdown hanging #94468

merged 8 commits into from
Mar 16, 2023

Conversation

yonatanp
Copy link
Contributor

@yonatanp yonatanp commented Jun 30, 2022 •
edited by bedevere-bot
Loading

Added a test to reproduce the issue and a fix to the issue itself.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Jun 30, 2022 •
edited
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@yonatanp yonatanp mentioned this pull request Sep 20, 2022
Closed
@AlexWaygood
Copy link
Member

@gpshead, would you be able to take a look at this PR?

@AlexWaygood AlexWaygood requested a review from gpshead March 14, 2023 22:03
@yonatanp yonatanp force-pushed the fix-issue-94440 branch 2 times, most recently from eab6073 to c376732 Compare March 15, 2023 16:22
@AlexWaygood
Copy link
Member

(We prefer to avoid force pushes in the CPython repo, where possible -- they interact badly with the GitHub UI, making it hard to see what changed between commits. All PRs are squash-merged in CPython anyway, so there's no need to worry about having a messy commit history in this PR :)

@yonatanp
Copy link
Contributor Author

(We prefer to avoid force pushes in the CPython repo, where possible -- they interact badly with the GitHub UI, making it hard to see what changed between commits. All PRs are squash-merged in CPython anyway, so there's no need to worry about having a messy commit history in this PR :)

Yeah - totally agree. I had the unfortunate case where my news change commit was created using my work-account, instead of my personal account. This in turn blocked the merge as the CLA Signing check would fail. I'm counting on the squash to clear it all out in the end :)

@yonatanp
Copy link
Contributor Author

:sigh: I'm creating a bit of a mess with the news item thing.
I might need some expert help here to get it back on track.
The check of generated files up-to-date was failing, and I assumed it was because I edited the news item manually (where initially it was created by the blurb-it bot). So I created a new blurb with the bot, but now I have two news items. I will manually delete the new one and commit.
Still not sure what's then the issue with the failing "generated files" check.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 15, 2023 •
edited
Loading

Still not sure what's then the issue with the failing "generated files" check.

Probably just a fluke. If it happens again, ping me and I'll take a look.

The news entry looks good now!

@yonatanp
Copy link
Contributor Author

Probably just a fluke. If it happens again, ping me and I'll take a look.

Failed again. It seems to be consistent.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 15, 2023 •
edited
Loading

Probably just a fluke. If it happens again, ping me and I'll take a look.

Failed again. It seems to be consistent.

It appears as though the CI check is broken somehow. It is failing on all PRs currently, e.g.:

So: not your problem; don't worry about it :)

AlexWaygood added a commit to AlexWaygood/cpython that referenced this pull request Mar 15, 2023
@AlexWaygood
Exclude ceval.c from the C-analyzer tool
6da2198 
The "check if generated files are up to date" CI check appears to be currently failing on all PRs (but not on pushes to main) 

See, for example:
- python#94468
- python#94468
- python#102731

This appears to be because the C-analyzer tool doesn't like the `#line` directives introduced in python@70185de. I'm advised by the message printed to the terminal in https://github.com/python/cpython/actions/runs/4428706945/jobs/7768216988#step:14:84 that this is the appropriate short-term fix!

Cc. @ericsnowcurrently
@AlexWaygood AlexWaygood mentioned this pull request Mar 15, 2023
Merged
@AlexWaygood
Copy link
Member

It appears as though the CI check is broken somehow. It is failing on all PRs currently, e.g.:

And here's (hopefully) a fix:

@AlexWaygood AlexWaygood mentioned this pull request Mar 15, 2023
Closed
AlexWaygood added a commit that referenced this pull request Mar 15, 2023
@AlexWaygood
Exclude ceval.c from the C-analyzer tool (#102735)
215007b 
The "check if generated files are up to date" CI check appears to be currently failing on all PRs (but not on pushes to main) 

See, for example:
- #94468
- #94468
- #102731

This appears to be because the C-analyzer tool doesn't like the `#line` directives introduced in 70185de. I'm advised by the message printed to the terminal in https://github.com/python/cpython/actions/runs/4428706945/jobs/7768216988#step:14:84 that this is the appropriate short-term fix!
@AlexWaygood
Copy link
Member

All green again now @yonatanp!

gpshead
gpshead approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work debugging this. This looks good to me.

@gpshead gpshead added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Mar 16, 2023
@gpshead gpshead merged commit 2dc9463 into python:main Mar 16, 2023
@miss-islington
Copy link
Contributor

Thanks @yonatanp for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-102746 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 16, 2023
@yonatanp @AlexWaygood @miss-islington
pythongh-94440: Fix issue of ProcessPoolExecutor shutdown hanging (py…
55e607f 
…thonGH-94468)

Fix an issue of concurrent.futures ProcessPoolExecutor shutdown hanging.

(cherry picked from commit 2dc9463)

Co-authored-by: yonatanp <yonatan.perry@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Mar 16, 2023
@bedevere-bot
Copy link

GH-102747 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Mar 16, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 16, 2023
@yonatanp @AlexWaygood @miss-islington
pythongh-94440: Fix issue of ProcessPoolExecutor shutdown hanging (py…
7a62dae 
…thonGH-94468)

Fix an issue of concurrent.futures ProcessPoolExecutor shutdown hanging.

(cherry picked from commit 2dc9463)

Co-authored-by: yonatanp <yonatan.perry@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@gpshead gpshead self-assigned this Mar 16, 2023
miss-islington added a commit that referenced this pull request Mar 16, 2023
@miss-islington @yonatanp @AlexWaygood
gh-94440: Fix issue of ProcessPoolExecutor shutdown hanging (GH-94468)
9781240 
Fix an issue of concurrent.futures ProcessPoolExecutor shutdown hanging.

(cherry picked from commit 2dc9463)

Co-authored-by: yonatanp <yonatan.perry@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
miss-islington added a commit that referenced this pull request Mar 16, 2023
@miss-islington @yonatanp @AlexWaygood
gh-94440: Fix issue of ProcessPoolExecutor shutdown hanging (GH-94468)
0560fe3 
Fix an issue of concurrent.futures ProcessPoolExecutor shutdown hanging.

(cherry picked from commit 2dc9463)

Co-authored-by: yonatanp <yonatan.perry@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
carljm added a commit to carljm/cpython that referenced this pull request Mar 17, 2023
@carljm
Merge branch 'main' into inlinecomp2
1a8f4a0 
* main: (34 commits)
  pythongh-102701: Fix overflow in dictobject.c (pythonGH-102750)
  pythonGH-78530: add support for generators in `asyncio.wait` (python#102761)
  Increase stack reserve size for Windows debug builds to avoid test crashes (pythonGH-102764)
  pythongh-102755: Add PyErr_DisplayException(exc) (python#102756)
  Fix outdated note about 'int' rounding or truncating (python#102736)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (python#102760)
  pythongh-99726: Improves correctness of stat results for Windows, and uses faster API when available (pythonGH-102149)
  pythongh-102192: remove redundant exception fields from ssl module socket (python#102466)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (python#102743)
  pythongh-102737: Un-ignore ceval.c in the CI globals check (pythongh-102745)
  pythonGH-102748: remove legacy support for generator based coroutines from `asyncio.iscoroutine` (python#102749)
  pythongh-102721: Improve coverage of `_collections_abc._CallableGenericAlias` (python#102722)
  pythonGH-102653: Make recipe docstring show the correct distribution (python#102742)
  Add comments to `{typing,_collections_abc}._type_repr` about each other (python#102752)
  pythongh-102594: PyErr_SetObject adds note to exception raised on normalization error (python#102675)
  pythongh-94440: Fix issue of ProcessPoolExecutor shutdown hanging (python#94468)
  pythonGH-100112:  avoid using iterable coroutines in asyncio internally (python#100128)
  pythongh-102690: Use Edge as fallback in webbrowser instead of IE (python#102691)
  pythongh-102660: Fix Refleaks in import.c (python#102744)
  pythongh-102738: remove from cases generator the code related to register instructions (python#102739)
  ...
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
@AlexWaygood @Fidget-Spinner
Exclude ceval.c from the C-analyzer tool (python#102735)
7204c41 
The "check if generated files are up to date" CI check appears to be currently failing on all PRs (but not on pushes to main) 

See, for example:
- python#94468
- python#94468
- python#102731

This appears to be because the C-analyzer tool doesn't like the `#line` directives introduced in python@70185de. I'm advised by the message printed to the terminal in https://github.com/python/cpython/actions/runs/4428706945/jobs/7768216988#step:14:84 that this is the appropriate short-term fix!
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
@yonatanp @AlexWaygood @Fidget-Spinner
pythongh-94440: Fix issue of ProcessPoolExecutor shutdown hanging (py…
f9f542d 
…thon#94468)

Fix an issue of concurrent.futures ProcessPoolExecutor shutdown hanging.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@AlexWaygood @warsaw
Exclude ceval.c from the C-analyzer tool (python#102735)
abb9cbd 
The "check if generated files are up to date" CI check appears to be currently failing on all PRs (but not on pushes to main) 

See, for example:
- python#94468
- python#94468
- python#102731

This appears to be because the C-analyzer tool doesn't like the `#line` directives introduced in python@70185de. I'm advised by the message printed to the terminal in https://github.com/python/cpython/actions/runs/4428706945/jobs/7768216988#step:14:84 that this is the appropriate short-term fix!
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@yonatanp @AlexWaygood @warsaw
pythongh-94440: Fix issue of ProcessPoolExecutor shutdown hanging (py…
dc3eb06 
…thon#94468)

Fix an issue of concurrent.futures ProcessPoolExecutor shutdown hanging.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexWaygood AlexWaygood AlexWaygood left review comments

@gpshead gpshead gpshead approved these changes

Assignees

@gpshead gpshead

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yonatanp @bedevere-bot @AlexWaygood @miss-islington @gpshead