Remove gevent

[mherrmann]

The current implementation fails to forward messages received from the server to a callback registered via some_hub.client.on('method', callback). The reason for this is that the listener which polls for incoming messages is spawned via gevent.spawn(listener), but (at least in the source code of this repository) no other method such as gevent.joinall() is ever called. This results in the listener never actually running. This pull request replaces gevent by standard Python threads to fix the problem. As a result, the dependency on gevent can be dropped.

[u-abramchuk]

Hi,

thank you for the pull request.

The main reason to use gevent was its better resources utilization comparing to threads (as we used it with locust.io for load testing out main application web APIs).

During the early phases of signalr-client-py development we've faced the similar issue. The solution was to enable gevent monkey patching like

from gevent import monkey
monkey.patch_socket()

Let me know if that's the case. If yes I'll add add gevent monkey patching to init file to be done automatically when referenced.

Please share you thoughts.

[mherrmann]

Hi,

you're welcome - thank you for sharing your code to begin with!

I have never used gevent before but I can see why you had the need for it.

I don't think enabling gevent monkey patching will help. I tried the following:

>>> from gevent import monkey; monkey.patch_all()
>>> import gevent
>>> def listener():
...   print('Listener ran!')
... 
>>> greenlet = gevent.spawn(listener)
>>> 
>>> # Nothing happens until we call the following line:
... 
>>> gevent.joinall([greenlet])
Listener ran!
[<Greenlet at 0x101c3c178>]

I strongly suspect that your load testing script calls gevent.joinall(...) (or a function with a similar effect) somewhere. Either way, without any such call the current version of signalr-client-py never processes messages sent by the server.

What would you think of the following suggestion: See if gevent is available, if yes use it. Otherwise fall back to Python Threads:

try:
    import gevent
except ImportError:
    # gevent not available. Resort to Threads.

This way, your code would continue to run and mine would run also. Also, the fact that gevent isn't totally easy to install (requires some headers, and the correct version for Python 3) would make your library easier to use for what I would argue is the more common use case.

[u-abramchuk]

Hi,

I see now what you mean. There is no context switch in your sample. We used explicit calls to gevent.sleep in our tests. But maybe we should consider alternative approaches. I'll look into the issue deeper and let you know of results.

[u-abramchuk]

Hi,

I've created an example signalr application (ASP.NET vNext chat) as well as python client for it. I'll post some instructions on how to launch it later. I've also moved gevent monkey.patch_socket() to module's __init__.py file.

It illustrates the way we use signalr client now. As you can see there is no need for you to call something like gevent.joinall or gevent.wait. The only purpose of the connection.wait(1) call in the end of the script is to ensure that all the handlers have been called before the script finished. And you won't need it at all if you signalr connection lives as long as the whole application does.

Please let me know your thoughts on the matter. Thank you.

[mherrmann]

Hi,

firstly, thank you for mentioning me as a contributor! Also, thank you for asking my opinion. I do want to answer, but in the interest of the project I will be very honest. I hope you don't mind: I think it's a mistake to make gevent a hard requirement. You are forcing users like me who do not have performance concerns to install a dependency that is not easy to handle. Further, requiring users to call connection.wait(1) in at least some situations is a hassle. I think signalr-client-py is firstly a SignalR client for Python, and only then something that uses gevent. I believe many users of your library do not care what is used in the background.

Having said that, it's of course your project and completely up to you what to make of it. If you want my feedback, I would suggest making gevent optional and using threads by default. I am using my fork with threads in production and am very happy with it.

Take care!
M

[PawelTroka]

@mherrmann I have made a fork based on your changes
https://github.com/PawelTroka/signalr-client-threads

[mherrmann]

@PawelTroka cool! :)