Skip to content

Ensure ThreadPool is closed in setup_helpers #3548

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
merged 2 commits into from
Dec 13, 2021
Merged

Conversation

bobbyi
Copy link
Contributor

@bobbyi bobbyi commented Dec 12, 2021

The ParallelCompile setup helper using a ThreadPool to enable its parallelism. It does not properly close the pool when it is done with it.

This can lead to a Exception ignored in: <function Pool.__del__... message with traceback being printed at shutdown.

Suggested changelog entry:

* Ensure ThreadPool is closed in setup_helpers.

@bobbyi bobbyi requested a review from henryiii as a code owner December 12, 2021 17:47
The ParallelCompile setup helper using a ThreadPool to enable its
parallelism. It does not properly close the pool when it is done with
it.
This can lead to a "Exception ignored in: <function Pool.__del__..."
message with traceback being printed at shutdown.
Use pool.terminate() instead of context manager for Python 2.7
compatibility
@bobbyi
Copy link
Contributor Author

bobbyi commented Dec 12, 2021

@henryiii I just updated this PR because CI failed against Python 2.7. Turns out that the pool does not support being used as a context manager in that version. I've changed it to use a finally block that calls pool.terminate, which is what the pool's __exit__ method does

@rwgk
Copy link
Collaborator

rwgk commented Dec 13, 2021

I've not seen the "ICC latest" error before. Seems unrelated to this PR. Maybe coincidentally the compiler was just updated?

@henryiii
Copy link
Collaborator

henryiii commented Dec 13, 2021

I'd either add a note or maybe even use an if statement - we are very close to dropping Python 2 support (Jan 1) and long term I'd like the nicer syntax. There should be something making it easy to remember to clean up. "Python 2", "sys.verison_info" are a couple of things I'd normally grep when cleaning up.

@bobbyi
Copy link
Contributor Author

bobbyi commented Dec 13, 2021

@henryiii If it's only two weeks or so until Python 2 support is dropped, I'd lean towards reverting this back to the first version I had and having you not merge it until then.

@henryiii
Copy link
Collaborator

I'd like to get as many fixes as possible in before dropping Py 2. So how about we merge this one, then you could make a PR to the nicer version that we merge in 2 weeks? :)

@bobbyi
Copy link
Contributor Author

bobbyi commented Dec 13, 2021

@henryiii Sounds good. I've added a comment that mentions "Python 2". There are already couple others in this same file (related to Distutils using old-style class in Python 2) that can maybe be cleaned up at the same time as this one. This should be ready to merge.

@henryiii
Copy link
Collaborator

The note is fine. :) I'll be expecting the old-style class stuff, but won't be likely to remember this context manager simplification without the note.

@henryiii henryiii merged commit 7516811 into pybind:master Dec 13, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 13, 2021
@henryiii
Copy link
Collaborator

Please do not delete the changelog entry portion of the template. It fills our changelog. (fixed manually).

@henryiii
Copy link
Collaborator

Thanks!

@bobbyi bobbyi deleted the close_pool branch December 13, 2021 21:25
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Dec 21, 2021
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.

3 participants