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

Improve support for external event loops. #235

Merged
merged 1 commit into from
Nov 16, 2017

Conversation

joernheissler
Copy link

#72, #147, https://dev.eclipse.org/mhonarc/lists/paho-dev/msg03997.html and https://github.com/mossblaser/aiomqtt demonstrate that the current support for external event loops should be improved.

https://dev.eclipse.org/mhonarc/lists/paho-dev/msg03999.html suggests to add new callbacks to track the state of the library. That's what my change implements.

@TD22057
Copy link

TD22057 commented Oct 27, 2017

@joernheissler Just wanted to say thanks for doing this. This is going to be very useful. 👍

class AsyncioHelper:
def __init__(self, loop, client):
self.loop = loop
self.client = client = client
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to write this twice?

raise WouldBlockError()
raise

def _sock_send(self, buf):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this addresses issue #188 as well.

Copy link
Author

Choose a reason for hiding this comment

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

I don't really understand #188; I don't see the relation to my change. Maybe I looked at the wrong code lines?

client.subscribe(topic)

def on_message(self, client, userdata, msg):
if self.state not in {1,3,5}:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to see the positive case first, so that the else clause doesn't mean "not not in {...}".

Copy link
Author

Choose a reason for hiding this comment

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

I removed the "else". I prefer to have a program flow where the "good" case is always on the top indention level and all the bad cases just exit the function as soon as possible. "if this is bad, abort. If that is bad, abort. What's remaining must be good."

@0181532686cf4a31163be0bf3e6bb6732bf

Any chance that would be merged and included in next release?

Add four more callbacks and documentation to make it possible to use external
event loops other than select(). This could e.g. be asyncio, twisted or gevent.

  * on_socket_open - called when a new socket was opened. --> watch read
  * on_socket_close - called when the socket is about to be closed. --> unwatch read
  * on_socket_register_write - called when mqtt wants to write. --> watch write
  * on_socket_unregister_write - called when mqtt is finished writing. --> unwatch write

Add examples how to write external event loops:

  * loop_select.py - select() based event loop, works without the other changes.
  * loop_asyncio.py - asyncio based event loop, utilizes the new callbacks.

This should also fix the issues eclipse#72 and eclipse#147.

Signed-off-by: Jörn Heissler <eclipse.org@wulf.eu.org>
@joernheissler
Copy link
Author

I made some minor improvements to the documentation, and loop_misc in the asyncio example is now started from a callback.

@PierreF
Copy link
Contributor

PierreF commented Nov 6, 2017

Thanks for this PR, it add good support for asyncio to paho-mqtt.

While reviewing it, I'm wondering if our multiple loops support is not too complicated. My view is that

  • loop_start: is the easiest, user don't care about details of how it works
  • loop_forever: is easy also, user want to wait for completion
  • custom loop based on loop_read/loop_write/loop_misc: complex solution, not very tested (it's with this PR that correct support for asyncio arrive). The use-case (in my view) is to use asyncio with lots of connection.

My question is then: should we keep all those loops ? What if the default loop is an
asyncio loop ?

  • This would render custom loop useless
  • loop_forever could be a call to asyncio.run_until_complete
  • loop_start could be a thread that call loop_forever

Did you see any issue with going that way ? Any use case that require a custom base loop that a default asyncio won't match ?

@joernheissler
Copy link
Author

My question is then: should we keep all those loops ? What if the default loop is an asyncio loop?

Asyncio was only added in py 3.4. Unless you want to anger your users (#204) you shouldn't depend on asyncio. Personally I don't care about py < 3.5, but I suspect others do.

The socket() and want_write() methods could be removed in my opinion. Instead, a select style loop can use the new callbacks and keep two lists of file descriptors that need watching (reading+writing). If a callback is called, those lists are modified.

What you could do too is move the existing loops from the Client class to an extra module and perhaps keep a stub method with the old signature so existing code won't break. Those loops would use the new callbacks.

@PierreF
Copy link
Contributor

PierreF commented Nov 16, 2017

I though that asyncio existed on PyPI for older Python... but that not the case. It could be possible to fallback on select based loop that being said.
I will merge this PR and see later if the existing loop could make use of the new callbacks.
Thanks for your work and patience !

@PierreF PierreF merged commit 0b2b479 into eclipse:develop Nov 16, 2017
@jamesmyatt
Copy link
Contributor

The develop branch only supports Python 2.7 or 3.4+, and you can use https://github.com/haypo/trollius for Python 2.7 for asyncio.

It certainly feels like supporting the standard Python mechanism for asynchronous I/O would be a significant improvement.

@jamesmyatt
Copy link
Contributor

Actually, looks like trollius is abandoned and probably wouldn't be very helpful anyway since it would lack the keyword support.

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.

5 participants