ref(uwsgi): Warn if uWSGI is set up without proper thread support (#2738)

Author: sentrivana

sentry_sdk/_compat.py

@@ -140,29 +140,74 @@ def __new__(metacls, name, this_bases, d):
     return type.__new__(MetaClass, "temporary_class", (), {})
 
 
-def check_thread_support():
-    # type: () -> None
+def check_uwsgi_thread_support():
+    # type: () -> bool
+    # We check two things here:
+    #
+    # 1. uWSGI doesn't run in threaded mode by default -- issue a warning if
+    #    that's the case.
+    #
+    # 2. Additionally, if uWSGI is running in preforking mode (default), it needs
+    #    the --py-call-uwsgi-fork-hooks option for the SDK to work properly. This
+    #    is because any background threads spawned before the main process is
+    #    forked are NOT CLEANED UP IN THE CHILDREN BY DEFAULT even if
+    #    --enable-threads is on. One has to explicitly provide
+    #    --py-call-uwsgi-fork-hooks to force uWSGI to run regular cpython
+    #    after-fork hooks that take care of cleaning up stale thread data.
     try:
         from uwsgi import opt  # type: ignore
     except ImportError:
-        return
+        return True
+
+    from sentry_sdk.consts import FALSE_VALUES
+
+    def enabled(option):
+        # type: (str) -> bool
+        value = opt.get(option, False)
+        if isinstance(value, bool):
+            return value
+
+        if isinstance(value, bytes):
+            try:
+                value = value.decode()
+            except Exception:
+                pass
+
+        return value and str(value).lower() not in FALSE_VALUES
 
     # When `threads` is passed in as a uwsgi option,
     # `enable-threads` is implied on.
-    if "threads" in opt:
-        return
+    threads_enabled = "threads" in opt or enabled("enable-threads")
+    fork_hooks_on = enabled("py-call-uwsgi-fork-hooks")
+    lazy_mode = enabled("lazy-apps") or enabled("lazy")
 
-    # put here because of circular import
-    from sentry_sdk.consts import FALSE_VALUES
+    if lazy_mode and not threads_enabled:
+        from warnings import warn
 
-    if str(opt.get("enable-threads", "0")).lower() in FALSE_VALUES:
+        warn(
+            Warning(
+                "IMPORTANT: "
+                "We detected the use of uWSGI without thread support. "
+                "This might lead to unexpected issues. "
+                'Please run uWSGI with "--enable-threads" for full support.'
+            )
+        )
+
+        return False
+
+    elif not lazy_mode and (not threads_enabled or not fork_hooks_on):
         from warnings import warn
 
         warn(
             Warning(
-                "We detected the use of uwsgi with disabled threads.  "
-                "This will cause issues with the transport you are "
-                "trying to use.  Please enable threading for uwsgi.  "
-                '(Add the "enable-threads" flag).'
+                "IMPORTANT: "
+                "We detected the use of uWSGI in preforking mode without "
+                "thread support. This might lead to crashing workers. "
+                'Please run uWSGI with both "--enable-threads" and '
+                '"--py-call-uwsgi-fork-hooks" for full support.'
             )
         )
+
+        return False
+
+    return True

sentry_sdk/client.py

@@ -4,7 +4,13 @@
 import random
 import socket
 
-from sentry_sdk._compat import datetime_utcnow, string_types, text_type, iteritems
+from sentry_sdk._compat import (
+    datetime_utcnow,
+    string_types,
+    text_type,
+    iteritems,
+    check_uwsgi_thread_support,
+)
 from sentry_sdk.utils import (
     capture_internal_exceptions,
     current_stacktrace,
@@ -18,7 +24,7 @@
 )
 from sentry_sdk.serializer import serialize
 from sentry_sdk.tracing import trace, has_tracing_enabled
-from sentry_sdk.transport import make_transport
+from sentry_sdk.transport import HttpTransport, make_transport
 from sentry_sdk.consts import (
     DEFAULT_MAX_VALUE_LENGTH,
     DEFAULT_OPTIONS,
@@ -249,28 +255,15 @@ def _capture_envelope(envelope):
 
             self.metrics_aggregator = None  # type: Optional[MetricsAggregator]
             experiments = self.options.get("_experiments", {})
-            if experiments.get("enable_metrics", True) or experiments.get(
-                "force_enable_metrics", False
-            ):
-                try:
-                    import uwsgi  # type: ignore
-                except ImportError:
-                    uwsgi = None
-
-                if uwsgi is not None and not experiments.get(
-                    "force_enable_metrics", False
-                ):
-                    logger.warning("Metrics currently not supported with uWSGI.")
-
-                else:
-                    from sentry_sdk.metrics import MetricsAggregator
-
-                    self.metrics_aggregator = MetricsAggregator(
-                        capture_func=_capture_envelope,
-                        enable_code_locations=bool(
-                            experiments.get("metric_code_locations", True)
-                        ),
-                    )
+            if experiments.get("enable_metrics", True):
+                from sentry_sdk.metrics import MetricsAggregator
+
+                self.metrics_aggregator = MetricsAggregator(
+                    capture_func=_capture_envelope,
+                    enable_code_locations=bool(
+                        experiments.get("metric_code_locations", True)
+                    ),
+                )
 
             max_request_body_size = ("always", "never", "small", "medium")
             if self.options["max_request_body_size"] not in max_request_body_size:
@@ -316,6 +309,16 @@ def _capture_envelope(envelope):
 
         self._setup_instrumentation(self.options.get("functions_to_trace", []))
 
+        if (
+            self.monitor
+            or self.metrics_aggregator
+            or has_profiling_enabled(self.options)
+            or isinstance(self.transport, HttpTransport)
+        ):
+            # If we have anything on that could spawn a background thread, we
+            # need to check if it's safe to use them.
+            check_uwsgi_thread_support()
+
     @property
     def dsn(self):
         # type: () -> Optional[str]

sentry_sdk/consts.py

@@ -46,7 +46,6 @@
             "transport_zlib_compression_level": Optional[int],
             "transport_num_pools": Optional[int],
             "enable_metrics": Optional[bool],
-            "force_enable_metrics": Optional[bool],
             "metrics_summary_sample_rate": Optional[float],
             "should_summarize_metric": Optional[Callable[[str, MetricTags], bool]],
             "before_emit_metric": Optional[Callable[[str, MetricTags], bool]],

sentry_sdk/hub.py

@@ -1,6 +1,5 @@
 import copy
 import sys
-
 from contextlib import contextmanager
 
 from sentry_sdk._compat import with_metaclass

sentry_sdk/worker.py

@@ -2,7 +2,6 @@
 import threading
 
 from time import sleep, time
-from sentry_sdk._compat import check_thread_support
 from sentry_sdk._queue import Queue, FullError
 from sentry_sdk.utils import logger
 from sentry_sdk.consts import DEFAULT_QUEUE_SIZE
@@ -21,7 +20,6 @@
 class BackgroundWorker(object):
     def __init__(self, queue_size=DEFAULT_QUEUE_SIZE):
         # type: (int) -> None
-        check_thread_support()
         self._queue = Queue(queue_size)  # type: Queue
         self._lock = threading.Lock()
         self._thread = None  # type: Optional[threading.Thread]

tests/test_client.py

@@ -5,8 +5,8 @@
 import subprocess
 import sys
 import time
-
 from textwrap import dedent
+
 from sentry_sdk import (
     Hub,
     Client,
@@ -1316,3 +1316,43 @@ def test_error_sampler(_, sentry_init, capture_events, test_config):
 
         # Ensure two arguments (the event and hint) were passed to the sampler function
         assert len(test_config.sampler_function_mock.call_args[0]) == 2
+
+
+@pytest.mark.forked
+@pytest.mark.parametrize(
+    "opt,missing_flags",
+    [
+        # lazy mode with enable-threads, no warning
+        [{"enable-threads": True, "lazy-apps": True}, []],
+        [{"enable-threads": "true", "lazy-apps": b"1"}, []],
+        # preforking mode with enable-threads and py-call-uwsgi-fork-hooks, no warning
+        [{"enable-threads": True, "py-call-uwsgi-fork-hooks": True}, []],
+        [{"enable-threads": b"true", "py-call-uwsgi-fork-hooks": b"on"}, []],
+        # lazy mode, no enable-threads, warning
+        [{"lazy-apps": True}, ["--enable-threads"]],
+        [{"enable-threads": b"false", "lazy-apps": True}, ["--enable-threads"]],
+        [{"enable-threads": b"0", "lazy": True}, ["--enable-threads"]],
+        # preforking mode, no enable-threads or py-call-uwsgi-fork-hooks, warning
+        [{}, ["--enable-threads", "--py-call-uwsgi-fork-hooks"]],
+        [{"processes": b"2"}, ["--enable-threads", "--py-call-uwsgi-fork-hooks"]],
+        [{"enable-threads": True}, ["--py-call-uwsgi-fork-hooks"]],
+        [{"enable-threads": b"1"}, ["--py-call-uwsgi-fork-hooks"]],
+        [
+            {"enable-threads": b"false"},
+            ["--enable-threads", "--py-call-uwsgi-fork-hooks"],
+        ],
+        [{"py-call-uwsgi-fork-hooks": True}, ["--enable-threads"]],
+    ],
+)
+def test_uwsgi_warnings(sentry_init, recwarn, opt, missing_flags):
+    uwsgi = mock.MagicMock()
+    uwsgi.opt = opt
+    with mock.patch.dict("sys.modules", uwsgi=uwsgi):
+        sentry_init(profiles_sample_rate=1.0)
+        if missing_flags:
+            assert len(recwarn) == 1
+            record = recwarn.pop()
+            for flag in missing_flags:
+                assert flag in str(record.message)
+        else:
+            assert not recwarn