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

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

Merged
merged 10 commits into from
Feb 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
52bb739
RuntimeError fix: Check for explicit runtime error message
Jibola Jan 25, 2024
8e91c9e
Added test that error fires for anything above version 3.12
Jibola Jan 30, 2024
ea7a37f
Leverage assertFalse; additional check for is_finalizing
Jibola Jan 31, 2024
4eb0212
set to 'intepreter shutdown'
Jibola Jan 31, 2024
43b0fd2
Merge branch 'master' into PYTHON-4147
Jibola Feb 1, 2024
c139c8b
updated the changelog.rst language
Jibola Feb 1, 2024
deaf3ee
Update doc/changelog.rst
Jibola Feb 1, 2024
40a9fec
fix D000 lint-manual error
Jibola Feb 2, 2024
bfbbb4f
Merge branch 'PYTHON-4147' of https://github.com/Jibola/mongo-python-…
Jibola Feb 2, 2024
f6b3fcf
fixed merge conflicts
Jibola Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Leverage assertFalse; additional check for is_finalizing
  • Loading branch information
@Jibola
Jibola committed Jan 31, 2024
commit ea7a37fd9625ebf15ace463f4ae8601e8cd6cc48
3 changes: 2 additions & 1 deletion pymongo/periodic_executor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

from __future__ import annotations

import sys
import threading
import time
import weakref
Expand Down Expand Up @@ -98,7 +99,7 @@ def open(self) -> None:
try:
thread.start()
except RuntimeError as e:
if str(e) == _THREAD_START_ON_SHUTDOWN_ERR:
if _THREAD_START_ON_SHUTDOWN_ERR in str(e) or sys.is_finalizing():
self._thread = None
return
raise
Expand Down
6 changes: 3 additions & 3 deletions test/test_monitor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,16 @@ def test_cleanup_executors_on_client_close(self):
for executor in executors:
wait_until(lambda: executor._stopped, f"closed executor: {executor._name}", timeout=5)

@unittest.skipIf(sys.version_info[:2] >= (3, 12), reason="Python version must be (>=3.12)")
def test_no_thread_start_runtime_err_on_shutdown(self):
"""Test we silence noisy runtime errors fired when the MongoClient spawns a new thread
on process shutdown."""
command = ["python", "-c", "'from pymongo import MongoClient; c = MongoClient()'"]
command = [sys.executable, "-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 •
edited
Loading

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.

)

assert not completed_process.stderr
self.assertFalse(completed_process.stderr)
self.assertFalse(completed_process.stdout)


if __name__ == "__main__":
Expand Down
Loading