nikipore / stompest

STOMP client library for Python including both synchronous and Twisted implementations.
Other
76 stars 47 forks source link

gevent support #11

Open dantman opened 11 years ago

dantman commented 11 years ago

It would be nice to have explicit gevent support. Right now stompest will work fine in gevent if you monkey patch python's socket and select. But it would be nice to have stompest work without using monkey patching.

Here's a demo script showing stompest with gevent: https://gist.github.com/dantman/5290438

Running that script right now will work due to the monkey patch lines. But see what happens when you comment out the two monkey.patch_*() lines.

Ideally stompest would either have explicit gevent support in the sync client or there would be a stompest.gevent that could be imported instead of stompest.sync.

nikipore commented 11 years ago

Very nice and clean example. It's impressive to see how easy gevent can be sneaked into synchronous code. I already thought of adding an issue for gevent support myself, basically to teach myself a bit of gevent, so you are preaching to the choir. But I do not have the time for this right now, and I wouldn't be using it in a serious real-life application. I also would have looked into trying to build a unified asynchronous client supporting both Twisted and gevent, but that's probably not the way gevent works. To cut a long story short, I'd be happy if you added gevent support to stompest in the way you see fit.

I find a solution with a stompest.gevent the cleanest approach. This would be analogous to stompest.async, meaning that stompest.gevent would be deployed as a separate PyPI package. This keeps the stompest core self-consistent and lean, plain Python. Too many people would be scared off by big and sometimes problematic packages like Twisted or gevent. The setup should be straightforward if you follow along the lines of stompest.async. You basically have to copy and paste /src/async to /src/gevent and you are good to go. I am happy to help if you need it. I would prefer if all modules stayed in this repository for now, because I find it easier to apply upgrades across packages and have the documentation for all stompest packages centralized in one place.

If you don't feel like contributing right now, I would still be glad to add a section with your gevent example to the documentation.

dantman commented 11 years ago

I'm fine with the idea of requiring stompest.gevent but a whole new package seems complete overkill for this.

The reality is that the only thing needed for gevent to work without monkey patching is for stompest.sync.transport to use gevent.select instead of select for two lines in canRead and use gevent.socket instead of socket for one line inside of connect.

Everything else is 100% the same as sync. It feels very non-DRY to have an actual separate package.

Maybe if we modified the base of the sync code so we have an abstract transport that doesn't actually understand how to use socket or select. Then make sync subclass that transport with one using socket and select. And a stompest.gevent can make a transport subclass that uses gevent.socket and gevent.select.

nikipore commented 11 years ago

We could add socket and select as factories to the StompFrameTransport (analogous to the parser factory which should be renamed, then), with the default implementations standard Python socketand select.

It does seem like overkill, but do you find it acceptable to import gevent in the stompest core PyPI package without stating in its description that there is a dependency to gevent? If so, I'm fine with that. By the way, there is no violation of DRY in the code at all in either scenario, it's more a question of deployment and dependency handling.

dantman commented 11 years ago

This might not be as easy as I thought. stompest.sync.client.Stomp uses time.sleep which also needs to use a gevent version.