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

fix(wsgi): Log SystemExit (with non-zero exit code) and KeyboardInterrupt #380

Merged
merged 9 commits into from
Jun 1, 2019

Conversation

saifelse
Copy link
Contributor

@saifelse saifelse commented May 30, 2019

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:

Fixes: GH-379

There are notably 3 places an exception could be raised, but it looks like we're only
testing one case. If we want to test the other two, would appreciate advice 🙏🏾.

…I 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: getsentryGH-379
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Instead of having so many duplicated except blocks, could you catch BaseException instead of Exception everywhere, and have the special logic for SystemExit in _capture_exception?

Other than that lgtm

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.
@saifelse saifelse changed the title Log SystemExit (with non-zero exit code) and KeyboardInterrupt in WSGI middleware fix(wsgi): Log SystemExit (with non-zero exit code) and KeyboardInterrupt May 31, 2019
@saifelse
Copy link
Contributor Author

@untitaker : thanks, back to you!

Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one change necessary, also please make sure that CI passes (see CONTRIBUTING.md)

@@ -192,8 +197,8 @@ def close(self):
self._response.close()
except AttributeError:
pass
except Exception:
reraise(*_capture_exception(self._hub))
except BaseException:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing an as e here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof, thanks

@saifelse
Copy link
Contributor Author

saifelse commented May 31, 2019

one change necessary, also please make sure that CI passes (see CONTRIBUTING.md)

looks like it was failing from zeus, so hopefully merging in master takes care of that 🤞🏾

@untitaker untitaker merged commit ddab2d4 into getsentry:master Jun 1, 2019
@untitaker
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SystemExit and KeyboardInterrupt no longer caught (regression from raven-python)
2 participants