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
12 changes: 11 additions & 1 deletion pymongo/periodic_executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@

from pymongo.lock import _create_lock

_THREAD_START_ON_SHUTDOWN_ERR = "can't create new thread at interpreter shutdown"


class PeriodicExecutor:
def __init__(
Expand Down Expand Up @@ -91,7 +93,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 str(e) == _THREAD_START_ON_SHUTDOWN_ERR:
Jibola marked this conversation as resolved.
Show resolved Hide resolved
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)

@unittest.skipIf(sys.version_info[:2] >= (3, 12), reason="Python version must be (>=3.12)")
Jibola marked this conversation as resolved.
Show resolved Hide resolved
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()'"]
Jibola marked this conversation as resolved.
Show resolved Hide resolved
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.

)

assert not completed_process.stderr
Jibola marked this conversation as resolved.
Show resolved Hide resolved


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