Cue / scales

scales - Metrics for Python
Apache License 2.0
920 stars 73 forks source link

Gevent compatablity #8

Closed NorthIsUp closed 12 years ago

NorthIsUp commented 12 years ago

When using gevent the flask server would block if the wrong event module was imported.

I actually wrote a few other implementations on how to do this including not subclassing Thread, but just using Thread.spawn. While using Thread.spawn is more compatible I think, this was the smaller change. Thoughts?

kennethreitz commented 12 years ago

Why wouldn't you be using gunicorn's gevent workers? I think that would make this problem go away.

NorthIsUp commented 12 years ago

First, I am using gevent workers. But why would using a gunicorn worker be different than just doing your own monkey patch. Shouldn't the library work in either case?

It appears the the core issue is how gevent patches Event and Thread. But I didn't dive to deeply in.

kennethreitz commented 12 years ago

I'm asking because I'm curious :)

Does the monkeypatching not cover Thread?

NorthIsUp commented 12 years ago

It does, but I've found that it works well with things like Thread.spawn() and less well with class foo(Thread). If that makes sense.

from their docs:

Monkey can also patch thread and threading to become greenlet-based. So :func:`thread.start_new_thread`
starts a new greenlet instead and :class:`threading.local` becomes a greenlet-local storage.

These are the sources that are used for the monkey patching: https://bitbucket.org/denis/gevent/src/6dcc7294dc3d/gevent/monkey.py https://bitbucket.org/denis/gevent/src/6dcc7294dc3d/gevent/thread.py https://bitbucket.org/denis/gevent/src/6dcc7294dc3d/gevent/event.py

NorthIsUp commented 12 years ago

My intuition is that gevent is patching Thread correctly but not Event. So that is why the try/except on the import worked.

I have a pull request open on gevent for an 'is_patched(module)' method. If that gets in this will be an easy fix.

robbywalker commented 12 years ago

Thanks for the change! My concern is whether this will cause issues for users who have gevent available (so the import succeeds) but aren't using it in the project at hand. Is that a valid concern?

NorthIsUp commented 12 years ago

Yes.

I currently have this pull request open on gevent which adds an "is_patched" method which could then be used. I'll try to re-write the scales patch to eliminate the need to do any gevent stuff tho, as it really should be written in a framework agnostic method. Like what about eventlet or twisted? they may have similar issues.

https://bitbucket.org/denis/gevent/pull-request/11/added-monkeyis_module_patched-and

NorthIsUp commented 12 years ago

I submitted a new request here: https://github.com/Greplin/scales/pull/10