From f8213b85bef6753f51a8e4cdf6ee1b1e8d064c42 Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Thu, 30 May 2019 05:25:32 -0700 Subject: [PATCH 1/8] Log SystemExit (with non-zero exit code) and KeyboardInterrupt in WSGI middleware Neither SystemExit nor KeyboardInterrupt subclass Exception, so we must explicitly handle these two exception classes in addition to Exception in the WSGI middleware. This is notably a direct port from raven-python: - https://github.com/getsentry/raven-python/blob/master/raven/middleware.py - https://github.com/getsentry/raven-python/blob/master/tests/middleware/tests.py Fixes: GH-379 --- sentry_sdk/integrations/wsgi.py | 17 ++++++- tests/integrations/wsgi/test_wsgi.py | 75 ++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index f91f10ac67..e8b2dd48a6 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -89,6 +89,10 @@ def __call__(self, environ, start_response): rv = self.app(environ, start_response) except Exception: reraise(*_capture_exception(hub)) + except KeyboardInterrupt: + reraise(*_capture_exception(self._hub)) + except SystemExit as e: + reraise(*_capture_exception(self._hub, skip_capture=e.code == 0)) return _ScopedResponse(hub, rv) @@ -149,7 +153,7 @@ def get_client_ip(environ): return environ.get("REMOTE_ADDR") -def _capture_exception(hub): +def _capture_exception(hub, skip_capture=False): # type: (Hub) -> ExcInfo # Check client here as it might have been unset while streaming response if hub.client is not None: @@ -159,7 +163,8 @@ def _capture_exception(hub): client_options=hub.client.options, mechanism={"type": "wsgi", "handled": False}, ) - hub.capture_event(event, hint=hint) + if not skip_capture: + hub.capture_event(event, hint=hint) return exc_info @@ -183,6 +188,10 @@ def __iter__(self): break except Exception: reraise(*_capture_exception(self._hub)) + except KeyboardInterrupt: + reraise(*_capture_exception(self._hub)) + except SystemExit as e: + reraise(*_capture_exception(self._hub, skip_capture=e.code == 0)) yield chunk @@ -194,6 +203,10 @@ def close(self): pass except Exception: reraise(*_capture_exception(self._hub)) + except KeyboardInterrupt: + reraise(*_capture_exception(self._hub)) + except SystemExit as e: + reraise(*_capture_exception(self._hub, skip_capture=e.code == 0)) def _make_wsgi_event_processor(environ): diff --git a/tests/integrations/wsgi/test_wsgi.py b/tests/integrations/wsgi/test_wsgi.py index b63540c3f6..b1ddd2b542 100644 --- a/tests/integrations/wsgi/test_wsgi.py +++ b/tests/integrations/wsgi/test_wsgi.py @@ -1,3 +1,5 @@ +import logging + from werkzeug.test import Client import pytest @@ -12,6 +14,28 @@ def app(environ, start_response): return app +class IterableApp(object): + def __init__(self, iterable): + self.iterable = iterable + + def __call__(self, environ, start_response): + return self.iterable + + +class ExitingIterable(object): + def __init__(self, exc_func): + self._exc_func = exc_func + + def __iter__(self): + return self + + def __next__(self): + raise self._exc_func() + + def next(self): + return type(self).__next__(self) + + def test_basic(sentry_init, crashing_app, capture_events): sentry_init(send_default_pii=True) app = SentryWsgiMiddleware(crashing_app) @@ -30,3 +54,54 @@ def test_basic(sentry_init, crashing_app, capture_events): "query_string": "", "url": "http://localhost/", } + + +def test_systemexit_0_is_ignored(sentry_init, capture_events): + sentry_init(send_default_pii=True) + iterable = ExitingIterable(lambda: SystemExit(0)) + app = SentryWsgiMiddleware(IterableApp(iterable)) + client = Client(app) + events = capture_events() + + with pytest.raises(SystemExit): + client.get("/") + + assert len(events) == 0 + + +def test_systemexit_1_is_captured(sentry_init, capture_events): + sentry_init(send_default_pii=True) + iterable = ExitingIterable(lambda: SystemExit(1)) + app = SentryWsgiMiddleware(IterableApp(iterable)) + client = Client(app) + events = capture_events() + + with pytest.raises(SystemExit): + client.get("/") + + event, = events + + assert "exception" in event + exc = event["exception"]["values"][-1] + assert exc["type"] == "SystemExit" + assert exc["value"] == "1" + assert event["level"] == "error" + + +def test_keyboard_interrupt_is_captured(sentry_init, capture_events): + sentry_init(send_default_pii=True) + iterable = ExitingIterable(lambda: KeyboardInterrupt()) + app = SentryWsgiMiddleware(IterableApp(iterable)) + client = Client(app) + events = capture_events() + + with pytest.raises(KeyboardInterrupt): + client.get("/") + + event, = events + + assert "exception" in event + exc = event["exception"]["values"][-1] + assert exc["type"] == "KeyboardInterrupt" + assert exc["value"] == "" + assert event["level"] == "error" From 269e50f97afce31b057b7fbd7d0818bfce875053 Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Thu, 30 May 2019 05:55:06 -0700 Subject: [PATCH 2/8] Fix hub reference --- sentry_sdk/integrations/wsgi.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index e8b2dd48a6..5207b45ce9 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -90,9 +90,9 @@ def __call__(self, environ, start_response): except Exception: reraise(*_capture_exception(hub)) except KeyboardInterrupt: - reraise(*_capture_exception(self._hub)) + reraise(*_capture_exception(hub)) except SystemExit as e: - reraise(*_capture_exception(self._hub, skip_capture=e.code == 0)) + reraise(*_capture_exception(hub, skip_capture=e.code == 0)) return _ScopedResponse(hub, rv) From f26858f835daf0cf82b02de058886e4a0a3ba1ab Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Thu, 30 May 2019 20:44:47 -0700 Subject: [PATCH 3/8] Generalize to BaseException, also correctly handle SystemExit(None) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the python docs: > If the value is an integer, it specifies the system exit status (passed to C’s > exit() function); if it is None, the exit status is zero; if it has another > type (such as a string), the object’s value is printed and the exit status is > one. --- sentry_sdk/integrations/wsgi.py | 40 +++++++++++----------------- tests/integrations/wsgi/test_wsgi.py | 14 ++++++---- 2 files changed, 24 insertions(+), 30 deletions(-) diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index 5207b45ce9..57f0a53ad9 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -87,12 +87,8 @@ def __call__(self, environ, start_response): try: rv = self.app(environ, start_response) - except Exception: - reraise(*_capture_exception(hub)) - except KeyboardInterrupt: - reraise(*_capture_exception(hub)) - except SystemExit as e: - reraise(*_capture_exception(hub, skip_capture=e.code == 0)) + except BaseException as e: + reraise(*_capture_exception(hub, e)) return _ScopedResponse(hub, rv) @@ -153,17 +149,19 @@ def get_client_ip(environ): return environ.get("REMOTE_ADDR") -def _capture_exception(hub, skip_capture=False): +def _capture_exception(hub, e): # type: (Hub) -> ExcInfo # Check client here as it might have been unset while streaming response if hub.client is not None: exc_info = sys.exc_info() - event, hint = event_from_exception( - exc_info, - client_options=hub.client.options, - mechanism={"type": "wsgi", "handled": False}, - ) - if not skip_capture: + # SystemExit(0) is the only uncaught exception that is expected behavior + should_skip_capture = isinstance(e, SystemExit) and e.code in (0, None) + if not should_skip_capture: + event, hint = event_from_exception( + exc_info, + client_options=hub.client.options, + mechanism={"type": "wsgi", "handled": False}, + ) hub.capture_event(event, hint=hint) return exc_info @@ -186,12 +184,8 @@ def __iter__(self): chunk = next(iterator) except StopIteration: break - except Exception: - reraise(*_capture_exception(self._hub)) - except KeyboardInterrupt: - reraise(*_capture_exception(self._hub)) - except SystemExit as e: - reraise(*_capture_exception(self._hub, skip_capture=e.code == 0)) + except BaseException as e: + reraise(*_capture_exception(self._hub, e)) yield chunk @@ -201,12 +195,8 @@ def close(self): self._response.close() except AttributeError: pass - except Exception: - reraise(*_capture_exception(self._hub)) - except KeyboardInterrupt: - reraise(*_capture_exception(self._hub)) - except SystemExit as e: - reraise(*_capture_exception(self._hub, skip_capture=e.code == 0)) + except BaseException: + reraise(*_capture_exception(self._hub, e)) def _make_wsgi_event_processor(environ): diff --git a/tests/integrations/wsgi/test_wsgi.py b/tests/integrations/wsgi/test_wsgi.py index b1ddd2b542..3bcd25852a 100644 --- a/tests/integrations/wsgi/test_wsgi.py +++ b/tests/integrations/wsgi/test_wsgi.py @@ -56,9 +56,11 @@ def test_basic(sentry_init, crashing_app, capture_events): } -def test_systemexit_0_is_ignored(sentry_init, capture_events): +@pytest.fixture(params=[0, None]) +def test_systemexit_zero_is_ignored(sentry_init, capture_events, request): + zero_code = request.param sentry_init(send_default_pii=True) - iterable = ExitingIterable(lambda: SystemExit(0)) + iterable = ExitingIterable(lambda: SystemExit(zero_code)) app = SentryWsgiMiddleware(IterableApp(iterable)) client = Client(app) events = capture_events() @@ -69,9 +71,11 @@ def test_systemexit_0_is_ignored(sentry_init, capture_events): assert len(events) == 0 -def test_systemexit_1_is_captured(sentry_init, capture_events): +@pytest.fixture(params=["", "foo", 1, 2]) +def test_systemexit_nonzero_is_captured(sentry_init, capture_events, request): + nonzero_code = request.param sentry_init(send_default_pii=True) - iterable = ExitingIterable(lambda: SystemExit(1)) + iterable = ExitingIterable(lambda: SystemExit(nonzero_code)) app = SentryWsgiMiddleware(IterableApp(iterable)) client = Client(app) events = capture_events() @@ -84,7 +88,7 @@ def test_systemexit_1_is_captured(sentry_init, capture_events): assert "exception" in event exc = event["exception"]["values"][-1] assert exc["type"] == "SystemExit" - assert exc["value"] == "1" + assert exc["value"] == nonzero_code assert event["level"] == "error" From bf7c95d098901c40b8f4f7f5e329eb22f3d4d456 Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Thu, 30 May 2019 20:54:22 -0700 Subject: [PATCH 4/8] Fix _capture_exception crash if hub.client is None Crash introduced in GH-270 --- sentry_sdk/integrations/wsgi.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index 57f0a53ad9..1972d60e3f 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -151,9 +151,10 @@ def get_client_ip(environ): def _capture_exception(hub, e): # type: (Hub) -> ExcInfo + exc_info = sys.exc_info() + # Check client here as it might have been unset while streaming response if hub.client is not None: - exc_info = sys.exc_info() # SystemExit(0) is the only uncaught exception that is expected behavior should_skip_capture = isinstance(e, SystemExit) and e.code in (0, None) if not should_skip_capture: @@ -163,6 +164,7 @@ def _capture_exception(hub, e): mechanism={"type": "wsgi", "handled": False}, ) hub.capture_event(event, hint=hint) + return exc_info From b0629a921b69d181a3d97c3becd51006e7e788cd Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Fri, 31 May 2019 00:25:17 -0700 Subject: [PATCH 5/8] Fix passing exception in _ScopedResponse.close --- sentry_sdk/integrations/wsgi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index 1972d60e3f..ccf2115e9e 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -197,7 +197,7 @@ def close(self): self._response.close() except AttributeError: pass - except BaseException: + except BaseException as e: reraise(*_capture_exception(self._hub, e)) From badceda9ed5d01111ff52ac31587519547e426dd Mon Sep 17 00:00:00 2001 From: Saif Hakim Date: Fri, 31 May 2019 02:13:56 -0700 Subject: [PATCH 6/8] Fix lint --- sentry_sdk/integrations/wsgi.py | 2 +- tests/integrations/wsgi/test_wsgi.py | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index ccf2115e9e..fc18b9d5d0 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -150,7 +150,7 @@ def get_client_ip(environ): def _capture_exception(hub, e): - # type: (Hub) -> ExcInfo + # type: (Hub, BaseException) -> ExcInfo exc_info = sys.exc_info() # Check client here as it might have been unset while streaming response diff --git a/tests/integrations/wsgi/test_wsgi.py b/tests/integrations/wsgi/test_wsgi.py index 3bcd25852a..be26496e98 100644 --- a/tests/integrations/wsgi/test_wsgi.py +++ b/tests/integrations/wsgi/test_wsgi.py @@ -1,5 +1,3 @@ -import logging - from werkzeug.test import Client import pytest From 776d760a8b684191b07b5b00bc0ec82680eae48a Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Sat, 1 Jun 2019 16:22:14 +0200 Subject: [PATCH 7/8] ref: Remove unnecessary parameter from _capture_exception --- sentry_sdk/integrations/wsgi.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index fc18b9d5d0..67228541d4 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -87,8 +87,8 @@ def __call__(self, environ, start_response): try: rv = self.app(environ, start_response) - except BaseException as e: - reraise(*_capture_exception(hub, e)) + except BaseException: + reraise(*_capture_exception(hub)) return _ScopedResponse(hub, rv) @@ -149,12 +149,14 @@ def get_client_ip(environ): return environ.get("REMOTE_ADDR") -def _capture_exception(hub, e): - # type: (Hub, BaseException) -> ExcInfo +def _capture_exception(hub): + # type: (Hub) -> ExcInfo exc_info = sys.exc_info() # Check client here as it might have been unset while streaming response if hub.client is not None: + e = exc_info[2] + # SystemExit(0) is the only uncaught exception that is expected behavior should_skip_capture = isinstance(e, SystemExit) and e.code in (0, None) if not should_skip_capture: @@ -186,8 +188,8 @@ def __iter__(self): chunk = next(iterator) except StopIteration: break - except BaseException as e: - reraise(*_capture_exception(self._hub, e)) + except BaseException: + reraise(*_capture_exception(self._hub)) yield chunk @@ -197,8 +199,8 @@ def close(self): self._response.close() except AttributeError: pass - except BaseException as e: - reraise(*_capture_exception(self._hub, e)) + except BaseException: + reraise(*_capture_exception(self._hub)) def _make_wsgi_event_processor(environ): From 7dc58ca150c60a097dddf6d3e796a5cd04af6ce2 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Sat, 1 Jun 2019 16:30:11 +0200 Subject: [PATCH 8/8] Update wsgi.py --- sentry_sdk/integrations/wsgi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index 67228541d4..9a098b6697 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -155,7 +155,7 @@ def _capture_exception(hub): # Check client here as it might have been unset while streaming response if hub.client is not None: - e = exc_info[2] + e = exc_info[1] # SystemExit(0) is the only uncaught exception that is expected behavior should_skip_capture = isinstance(e, SystemExit) and e.code in (0, None)