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.

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

PYTHON-4147: Silence noisy thread.start() RuntimeError at shutdown #1486

Merged
merged 10 commits into from Feb 5, 2024

Conversation

Jibola
Copy link
Contributor

@Jibola Jibola commented Jan 25, 2024

No description provided.

@Jibola Jibola requested a review from a team as a code owner January 25, 2024 22:02
@Jibola Jibola requested review from NoahStapp and removed request for a team January 25, 2024 22:02
Copy link
Contributor

@NoahStapp NoahStapp left a comment

Choose a reason for hiding this comment

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

One clarification for my own understanding, otherwise good!

try:
thread.start()
except RuntimeError as e:
if str(e) == "can't create new thread at interpreter shutdown":
Copy link
Contributor

Choose a reason for hiding this comment

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

We're assuming this error message will remain constant, right?

Copy link
Contributor Author

@Jibola Jibola Jan 25, 2024

Choose a reason for hiding this comment

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

Yep. Worst case, we can also directly tie this check to version 3.12 if it becomes something that does change. Waiting on response to python/cpython#114570 and follow up based on that.

@NoahStapp NoahStapp self-requested a review January 25, 2024 22:35
@ShaneHarvey ShaneHarvey self-requested a review January 25, 2024 22:50
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

RuntimeError fix: Check for explicit runtime error message

Could you update the PR title to explain the higher level of what's being changed? Something like "Silence noisy thread.start() RuntimeError at shutdown" would be more enlightening. Also we should add this to the changelog.

pymongo/periodic_executor.py Outdated Show resolved Hide resolved
pymongo/periodic_executor.py Show resolved Hide resolved
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test for this?

@Jibola Jibola changed the title PYTHON-4147: RuntimeError fix: Check for explicit runtime error message PYTHON-4147: Silence noisy thread.start() RuntimeError at shutdown Jan 26, 2024
test/test_monitor.py Outdated Show resolved Hide resolved
test/test_monitor.py Outdated Show resolved Hide resolved
on process shutdown."""
command = ["python", "-c", "'from pymongo import MongoClient; c = MongoClient()'"]
completed_process: subprocess.CompletedProcess = subprocess.run(
" ".join(command), shell=True, capture_output=True
Copy link
Member

Choose a reason for hiding this comment

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

No need to join on command since run() accepts a list.

Copy link
Contributor Author

@Jibola Jibola Jan 31, 2024

Choose a reason for hiding this comment

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

It needs to be a string with shell=True specified. I've been attempting to capture the output, and shell=True has been the main way for me to capture that output. It seems to not capture all of the shell output when done without the shell=True.

I'm not sure why this is the case and haven't found much good material explaining why, but since these tests are scoped and explicitly defined, I'd argue it's fine to keep this way. The only difference I know is that shell=True uses whatever native sh the underlying system uses, environment variables and all.

If anyone does know why it's not capturing the output in either stderr/stdout when using shell=False that would be great.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Thanks for explaining.

test/test_monitor.py Outdated Show resolved Hide resolved
pymongo/periodic_executor.py Outdated Show resolved Hide resolved
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

One last thing is to update the changelog to describe this bugfix.

doc/changelog.rst Outdated Show resolved Hide resolved
@ShaneHarvey
Copy link
Member

Whoops, lint is failing:

doc/changelog.rst:31: D000 Explicit markup ends without a blank line; unexpected unindent.

doc/changelog.rst Outdated Show resolved Hide resolved
doc/changelog.rst Outdated Show resolved Hide resolved
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

LGTM

@Jibola Jibola merged commit 0f7e1b0 into mongodb:master Feb 5, 2024
72 of 75 checks passed
@Jibola Jibola deleted the PYTHON-4147 branch February 5, 2024 13:46
blink1073 pushed a commit to blink1073/mongo-python-driver that referenced this pull request Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants