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
11 changes: 10 additions & 1 deletion pymongo/periodic_executor.py
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 @@ -91,7 +92,15 @@ def open(self) -> None:
thread.daemon = True
self._thread = weakref.proxy(thread)
_register_executor(self)
thread.start()
# Mitigation to RuntimeError firing when thread starts on shutdown
# https://github.com/python/cpython/issues/114570
try:
ShaneHarvey marked this conversation as resolved.
Show resolved Hide resolved
thread.start()
except RuntimeError as e:
if "interpreter shutdown" in str(e) or sys.is_finalizing():
self._thread = None
return
raise

def close(self, dummy: Any = None) -> None:
"""Stop. To restart, call open().
Expand Down
12 changes: 12 additions & 0 deletions test/test_monitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from __future__ import annotations

import gc
import subprocess
import sys
from functools import partial

Expand Down Expand Up @@ -79,6 +80,17 @@ def test_cleanup_executors_on_client_close(self):
for executor in executors:
wait_until(lambda: executor._stopped, f"closed executor: {executor._name}", timeout=5)

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 = [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

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.

)

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


if __name__ == "__main__":
unittest.main()