Add socket event monitor capability #50

claws · 2015-06-22T14:29:14Z

This pull request adds the ZMQ socket event monitor capability to aiozmq.

I am using this in my system which uses ephemeral ports. I use it to detect client disconnects, then tear down the socket, use a discovery mechanism to resolve the new endpoint address and finally re-establish the socket.

asvetlov · 2015-06-22T18:51:23Z

aiozmq/interface.py

@@ -194,3 +215,36 @@ def msg_received(self, data):

        data is the multipart tuple of bytes with at least one item.
        """
+
+MonitorEvents = {zmq.EVENT_ACCEPTED: 'accepted',


Use enum here

asvetlov · 2015-06-22T18:59:48Z

I support the idea in general but propose a bit another interface.
I suggest adding event_received() callback just to ZmqProtocol.

ZmqTransport should have enable_monitor() and disable_monitor() methods, both returns None. Monitor socket handled internally by transport and destroyed on transport closing if present.

I believe hiding monitor socket from library user makes public API easy and clean.

claws · 2015-06-28T14:52:53Z

This change hides most of the event monitor details from the user. The event monitor can be enabled by calling enable_monitor on a transport. The monitor can be disable using disable_monitor. The monitor will also be shutdown when the socket being monitored is closed. Socket events are captured by an internal event monitor protocol that converts the binary event into a useful dict and then passes it to the protocol running the socket being monitored. This simplifies the developer API as they only need to deal with the ZmqProtocol class to obtain messages or events.

This took a while to get going because I encountered a confusing problem obtaining the socket event messages when running within the event loop. Using standard zmq was fine. It turns out I had to connect first and then bind second when using inproc:// transport within the event loop. Apparently something with the zmq.EVENTS is not being reported properly by libzmq internals in this particular case. I found this problem mentioned here.

claws · 2015-06-28T14:53:41Z

Looks like I have a few CI issues to resolve, I'll push a change to fix this soon.

claws · 2015-06-28T15:34:17Z

It looks like I have a CI problem remaining - "connection refused". I can't replicate this locally using OSX or Linux (Ubuntu 14.04).

Have you encountered issues that only show up on TravisCI? Can you recommend any debugging strategies for this kind of problem (otherwise, I'm stuck with trial and error)?

claws · 2015-06-29T04:32:24Z

It looks like the last CI issue I had was related to the TravisCI test environment using libzmq3. In my test environments I am using a more recent version of libzmq (libzmq4). I think this explains why it works in my test environments (OSX and linux on machines at home and at work) but not on TravisCI. The socket monitor capability was only really added in libzmq>=4 and pyzmq>=14.4.

I have added runtime checks so that the library remains compatible with operating on libzmq3 (though without a socket monitoring capability) and can provide the monitoring capability when operating on libzmq4.

It might be time to revise the .travis.yml file to add test environments that also use libzmq4 so that full test coverage can be maintained.

claws · 2015-06-30T02:15:58Z

I consider this change complete now and ready for your review.

Unfortunately the latest TravisCI builds indicate failure but I think this status is erroneous. Two builds succeed and then I pushed a docs-only change which resulted in a broken build. I subsequently pushed a trivial change to re-trigger a Travis rebuild and again this produced a failure but in a different builder. I consider this a TravisCI issue, not a problem with the docs change that was last delivered.

As owner of this repo you could manually re-trigger a TravisCI build to re-check the build status to confirm if this is a TravisCI issue or a real code issue. You can do this by opening the TravisCI page and clicking on the rebuild icon. Details can be found here.

claws · 2015-06-30T08:43:56Z

@asvetlov could you manually bump TravisCI to perform another build? See above comment for details.

asvetlov · 2015-06-30T08:58:58Z

aiozmq/interface.py

+
+        This method is only called when a socket monitor is enabled.
+
+        :param event: A dict containing the event description keys `event`,


I dislike dict (yes, it's supported by zmq.utils.monitor.parse_monitor_message() but we may repeat these 6 lines of code.
My suggestion is using namedtuple instead.

claws · 2015-06-30T13:57:38Z

It looks like TravisCI fell over again running this docs-only update. Perhaps you could manually re-trigger the TravisCI build again?

asvetlov · 2015-06-30T14:16:38Z

Eah, Travis is pretty unstable for the last year.

asvetlov · 2015-06-30T21:04:48Z

I like the PR but please write additional tests.
Run make cov and see coverage html output.

asvetlov · 2015-06-30T21:05:08Z

aiozmq/core.py

+
+    def msg_received(self, data):
+        if len(data) != 2 or len(data[0]) != 6:
+            raise RuntimeError(


Not test covered

asvetlov · 2015-06-30T21:07:56Z

aiozmq/core.py

@@ -693,6 +776,8 @@ def close(self):
    def _force_close(self, exc):
        if self._conn_lost:
            return
+        if self._monitor:
+            self.disable_monitor()


Not covered

claws · 2015-07-01T10:19:56Z

I have added test coverage for the monitor additions. While I was looking at the coverage output I noticed some areas that could be updated to improve the coverage so I did that too (e.g. test supplying an external zmq context, get_write_buffer_limits) to improve the existing test coverage. These were in the vicinity of the areas I was updating anyway.

Add socket event monitor capability

asvetlov · 2015-07-01T13:43:53Z

Thanks!

asvetlov · 2015-07-01T13:44:39Z

Would you take a look on adding events monitoring to stream API?

Add socket event monitor capability

dc46d7d

asvetlov reviewed Jun 22, 2015
View reviewed changes

hide monitor socket, simplify developer API

cee2497

fixing code style and CI problem

5be2505

Chris Laws added 4 commits June 29, 2015 09:29

Merge branch 'master' of https://github.com/claws/aiozmq into add_events

adc574d

trial workaround for problem observed in Travis test environment

5d38a81

add libzmq and pyzmq library version checks

07ff32a

cleanups

e308a7e

Chris Laws added 2 commits June 30, 2015 10:25

docs update

4a31db9

trivial change to bump Travis rebuild

86934fc

asvetlov reviewed Jun 30, 2015
View reviewed changes

claws added 2 commits June 30, 2015 20:40

convert event to a namedtuple

a876a01

update core.rst API docs

21bed81

asvetlov reviewed Jun 30, 2015
View reviewed changes

aiozmq/core.py

def msg_received(self, data):

if len(data) != 2 or len(data[0]) != 6:

raise RuntimeError(

Copy link

Member

asvetlov Jun 30, 2015

Choose a reason for hiding this comment

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

Not test covered

asvetlov reviewed Jun 30, 2015
View reviewed changes

claws and others added 3 commits July 1, 2015 07:32

Merge branch 'master' of https://github.com/claws/aiozmq into add_events

076f34e

increase test coverage

2fe4dea

fix style

5b6ae3c

asvetlov added a commit that referenced this pull request Jul 1, 2015

Merge pull request #50 from claws/add_events

4f48376

Add socket event monitor capability

asvetlov merged commit 4f48376 into aio-libs:master Jul 1, 2015

claws deleted the add_events branch July 2, 2015 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add socket event monitor capability #50

Add socket event monitor capability #50

claws commented Jun 22, 2015

asvetlov Jun 22, 2015

asvetlov commented Jun 22, 2015

claws commented Jun 28, 2015

claws commented Jun 28, 2015

claws commented Jun 28, 2015

claws commented Jun 29, 2015

claws commented Jun 30, 2015

claws commented Jun 30, 2015

asvetlov Jun 30, 2015

claws commented Jun 30, 2015

asvetlov commented Jun 30, 2015

asvetlov commented Jun 30, 2015

asvetlov Jun 30, 2015

asvetlov Jun 30, 2015

claws commented Jul 1, 2015

asvetlov commented Jul 1, 2015

asvetlov commented Jul 1, 2015


		This method is only called when a socket monitor is enabled.

		:param event: A dict containing the event description keys `event`,

Add socket event monitor capability #50

Add socket event monitor capability #50

Conversation

claws commented Jun 22, 2015

asvetlov Jun 22, 2015

Choose a reason for hiding this comment

asvetlov commented Jun 22, 2015

claws commented Jun 28, 2015

claws commented Jun 28, 2015

claws commented Jun 28, 2015

claws commented Jun 29, 2015

claws commented Jun 30, 2015

claws commented Jun 30, 2015

asvetlov Jun 30, 2015

Choose a reason for hiding this comment

claws commented Jun 30, 2015

asvetlov commented Jun 30, 2015

asvetlov commented Jun 30, 2015

asvetlov Jun 30, 2015

Choose a reason for hiding this comment

asvetlov Jun 30, 2015

Choose a reason for hiding this comment

claws commented Jul 1, 2015

asvetlov commented Jul 1, 2015

asvetlov commented Jul 1, 2015