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

[16.07] Do not instantiate the raven (sentry) client or tool conf watchdog threads until uWSGI postfork #2792

Merged
merged 3 commits into from Aug 16, 2016

Conversation

Projects
None yet
4 participants
@natefoo
Copy link
Member

commented Aug 15, 2016

As in #2774. I manually tested all three sentry access methods (log handler, exception handling middleware, explicit client capture) and they all seem to work this way.

The downside is that startup errors will no longer be sent to sentry under uWSGI. raven's Client does not give any access to the transport to call its stop() method so there's no proper way to start the client prefork, stop it just before forking, and then start it again once forking (if that would even work). Destroying the client object doesn't work either (unless explicit garbage collection is needed). In the end I felt like this is not a huge loss.

xref: getsentry/raven-python#806

@natefoo natefoo force-pushed the natefoo:uwsgi-fork-raven branch from 9579f55 to 59e88fa Aug 15, 2016

@natefoo natefoo changed the title [16.07] Do not instantiate the raven (sentry) client until uWSGI postfork [16.07] Do not instantiate the raven (sentry) client or tool conf watchdog threads until uWSGI postfork Aug 15, 2016

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2016

I rebased a commit on to this PR that also fixes the tool conf watchdogs.

ping @nsoranzo

@@ -151,7 +153,7 @@ def __init__(self, toolbox, observer_class):
self.start()

def start(self):
self.observer.start()
register_postfork_function(self.observer.start)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Aug 15, 2016

Member

I'd prefer lib/galaxy/tools/toolbox/watcher.py have nothing to do with uwsgi - in the abstract it really shouldn't and it is meant to be a general library utility useful outside a web server.

In this sense, I'd be much more comfortable with this if it was called:

lib/galaxy/util/threading.py

and register_postfork_function was called something else like handle_long_running_function.

This comment has been minimized.

Copy link
@natefoo

natefoo Aug 15, 2016

Author Member

I agree it shouldn't be uWSGI specific, which is why I put it in galaxy.util.postfork instead of something more uWSGI-specific. This is potentially a problem for any server that uses the traditional load-once-fork-many multiprocess model, not just uWSGI.

I think a name like handle_long_running_function would be a bit of a misnomer here since this is not really what it does. There could be a better name that indicates that this is a function that will be run "now or later depending on your multiprocess model".

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Aug 16, 2016

👍

@dannon

This comment has been minimized.

Copy link
Member

commented Aug 16, 2016

Yep, my sentry events are showing up too. +1

@dannon dannon merged commit a13117f into galaxyproject:release_16.07 Aug 16, 2016

4 checks passed

api test Build finished. 224 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 111 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 582 tests run, 0 skipped, 0 failed.
Details
@natefoo

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.