Support for nng_msg as Message #24

codypiersall · 2019-01-12T19:43:31Z

This is the beginning of supporting nng_msg in pynng. Related issue is #21.

So far only sychronous methods are supported. The API will change, I think, before this is merged; I think that my Message class will grow a public send() method. Instead of socket.sendmsg(msg), you would instead call msg.send(). Feedback welcome. To see how it's used right now, check out the tests!

goodboy

Lookin pretty good to me 👍

goodboy · 2019-01-13T08:05:01Z

pynng/nng.py

+        pipe_id = lib.nng_pipe_id(lib_pipe)
+        if pipe_id < 0:
+            # TODO: Better exception
+            raise Exception('No such pipe')


How about PipeNotFoundError much like ModuleNotFoundError?

First I want to check to see what nng itself does whenever it's given a pipe object that doesn't exist anymore, and see if it makes sense to reuse that exception; otherwise, I like this.

goodboy · 2019-01-13T08:12:05Z

test/test_msg.py

+to = 1000
+
+
+def test_socket_send_recv_msg():


goodboy · 2019-01-13T08:12:15Z

test/test_msg.py

+        msg = pipe.new_msg(b'oh hello friend')
+        assert isinstance(msg, pynng.Message)
+        assert msg.bytes == b'oh hello friend'
+        s1.sendmsg(msg)


I'm slightly partial to send_msg() / recv_msg() personally.

In my first version, I did use send_msg and recv_msg; I changed it to be more consistent with nng. But you convinced me! I'm going to change it back.

goodboy · 2019-01-13T08:13:57Z

test/test_msg.py

+        wait_pipe_len(s1, 1)
+        pipe = s1.pipes[0]
+        msg = pipe.new_msg(b'oh hello friend')
+        assert msg.pipe is pipe


Looks like you could probably just stick these 2 extra pipe assertions in the above test to save some duplication?

Probably worth doing. I was trying to narrow the scope of tests, but whenever the test is almost completely duplicated anyway it makes sense to put more into it.

goodboy · 2019-01-13T08:17:10Z

pynng/nng.py

+        flags = 0
+        if not block:
+            flags |= lib.NNG_FLAG_NONBLOCK
+        lib_sock = self.pipe.socket.socket


Hmm I wonder if it should be Socket.nng_socket (kinda out of scope)..

Yeah, I'm pretty unhappy with that on all the pynng right now. Listener has a listener attribute for the underlying nng object, Dialer has a dialer attribute, etc. I want to change it from what it is now, but am not sure whether to change it to either the nng library name (so nng_socket like you suggested here) or give every object a _lib_obj attribute that is the nng object.

I'm leaning toward the latter, but I'm not confident it's better. Feedback is welcome there!

Yeah that seems fine.
The only thing I can say helps me is some prefix that indicates what is ffi pointers / refs versus native python objects.

codypiersall · 2019-01-17T03:19:53Z

I was super wrong for giving the Message a send() method. I was thinking that a message would always need to have a pipe associated, but that's not the case. For example, it may not even be supported to set the pipe for pub/sub. So now socket will have a send_msg() method again.

I think that more or less wraps up Message support...

codypiersall · 2019-01-17T05:34:08Z

Most of the functionality is ironed out now, but the API is not really ready yet; mostly, I think I'll remove the new_msg() methods on Pipe, Socket, and Context, and just let people call Message() directly.

Additionally, I need to not do raise Exception() like I'm doing now, and I should think about adding threading.Lock() objects to Message to atomically check the _sent flag to prevent potential segfaults. I guess uncontested locks should be fast enough; on my computer it takes about 0.133 us to acquire and release a lock. (Command was python -m timeit -s 'from threading import Lock; l = Lock()' 'with l: pass')

So a little left to do on this PR, but not too much, I think.

Feedback welcome!

goodboy

Just realized this was already merged but figured I'd give it a once over anyway.
Had my head in my own pile of code this weekend, heh.

goodboy · 2019-01-21T03:15:14Z

pynng/nng.py

+        if pipe_id < 0:
+            # TODO: Better exception
+            raise Exception('No such pipe')
+        pipe = self._pipes[pipe_id]


return self._pipes[pipe_id] ?

goodboy · 2019-01-21T03:16:14Z

pynng/nng.py

+        with msg._mem_freed_lock:
+            msg._ensure_can_send()
+            with _aio.AIOHelper(self, self._async_backend) as aio:
+                val = await aio.asend_msg(msg)


return await aio.asend_msg(msg)

+1, this was cruft leftover from an earlier round of coding.

goodboy · 2019-01-21T03:20:17Z

pynng/nng.py

+            msg._ensure_can_send()
+            with _aio.AIOHelper(self, self._socket._async_backend) as aio:
+                val = await aio.asend_msg(msg)
+                return val


again just a single line seems fine.

goodboy · 2019-01-21T03:22:11Z

pynng/nng.py

+
+    def __init__(self, data, pipe=None):
+        self._pipe = pipe
+        # NB! There are two ways that a user can free resources that an nng_msg


This comment might be out of date now?
Re:

So the only way to send a message is with the send() and asend() methods on the object.

Good observation.

goodboy · 2019-01-21T03:29:20Z

pynng/nng.py

+    """
+
+    def __init__(self, data, pipe=None):
+        self._pipe = pipe


Maybe this should use the setter property?

Updating this also showed me a bug in my code: I thought you could "unset" a message's pipe by passing NULL, but I never tested it, and it caused problems. (I'm not sure why I ever thought that would be possible; nng_msg_set_pipe doesn't take a pointer, it takes a nng_pipe... oh well!)

goodboy · 2019-01-21T03:31:45Z

pynng/nng.py

+            self._pipe = None
+            return
+        if not isinstance(pipe, Pipe):
+            msg = 'pipe must be type Pipe, not {}'


msg = 'pipe must be type Pipe, not {}'.format(type(pipe))

I mean heck you could do it all in the ValueError arg.

goodboy · 2019-01-21T03:41:23Z

pynng/nng.py

+        self._pipe = pipe
+
+    @property
+    def buffer(self):


Would it maybe be handy to make this a context manager that will block you from being able to send while the buffer is in use?

I had the same idea, but I couldn't come up with an implementation that actually guaranteed preventing using a buffer after sending. Once the user has access to the buffer, there's no way to disable using it. Imagine code like this:

with msg.borrow_buffer() as buf: do_something_cool(buf) sock.send_msg(msg) # can't prevent this print(buf[0:10])

There is no way to mark a buffer as being invalid. This is a cffi.buffer type. Maybe it's not worth exposing it at all though? As far as I can tell this is the only place in the library where it would be pretty easy to cause a segfault. I named it _buffer with an underscore to indicate that it's dangerous, not that it's private; hopefully people would mostly reach for the bytes property...

goodboy · 2019-01-21T03:43:03Z

pynng/nng.py

+            if not self._mem_freed:
+                size = lib.nng_msg_len(self._nng_msg)
+                data = ffi.cast('char *', lib.nng_msg_body(self._nng_msg))
+                return ffi.buffer(data[0:size])


Do you need the 0?

Yeah, for ffi.buffer objects the start index has to be given.

codypiersall · 2019-01-21T14:31:36Z

Thanks for the feedback @tgoodlet! I'll look at it all soon.

Code cleanup based on review in #24 by @tgoodlet.

codypiersall added 2 commits January 12, 2019 13:38

WIP: Support for Message. Synchronous version works. API will change.

2075732

Add _test_util.py.

3775c0d

goodboy reviewed Jan 13, 2019

View reviewed changes

Rename recvmsg() to recv_msg and give Message a send() method.

cf4e203

codypiersall added 2 commits January 16, 2019 23:03

Implement async versions of send_msg and recv_msg on sockets.

a27e833

Implement async context message operations.

39b61b1

I think that more or less wraps up Message support...

codypiersall added 4 commits January 19, 2019 20:11

Make Message() constructor public.

42675ee

Make assertions about pipes attached to messages.

c87737c

Ensure messages cannot be double sent. (That would result in a segfault)

af8a716

Small cleanups, and renaming exception NoSuchFile to NoEntry.

0a8e4b1

codypiersall changed the title ~~WIP: Support for Message. Synchronous version works. API will change. Do not merge.~~ Support for nng_msg as Message Jan 21, 2019

codypiersall merged commit 0a8e4b1 into master Jan 21, 2019

goodboy reviewed Jan 21, 2019

View reviewed changes

codypiersall added a commit that referenced this pull request Jan 21, 2019

Fix bug when setting Message.pipe to None (and various cleanups).

29a60e6

Code cleanup based on review in #24 by @tgoodlet.

This was referenced Jan 21, 2019

Context needs recv_msg() synchronous function #29

Closed

Expose message objects as first class packets! #21

Closed

codypiersall deleted the nng_msg2 branch January 26, 2019 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for nng_msg as Message #24

Support for nng_msg as Message #24

codypiersall commented Jan 12, 2019

goodboy left a comment

goodboy Jan 13, 2019 •

edited

Loading

codypiersall Jan 13, 2019

goodboy Jan 13, 2019

goodboy Jan 13, 2019

codypiersall Jan 13, 2019

goodboy Jan 13, 2019

codypiersall Jan 13, 2019

goodboy Jan 13, 2019

codypiersall Jan 13, 2019

goodboy Jan 13, 2019 •

edited

Loading

codypiersall commented Jan 17, 2019

codypiersall commented Jan 17, 2019

goodboy left a comment

goodboy Jan 21, 2019

goodboy Jan 21, 2019

codypiersall Jan 21, 2019

goodboy Jan 21, 2019

goodboy Jan 21, 2019

codypiersall Jan 21, 2019

goodboy Jan 21, 2019

codypiersall Jan 21, 2019

codypiersall Jan 21, 2019

goodboy Jan 21, 2019

codypiersall Jan 21, 2019

goodboy Jan 21, 2019

codypiersall Jan 21, 2019

goodboy Jan 21, 2019

codypiersall Jan 21, 2019

codypiersall commented Jan 21, 2019

Support for nng_msg as Message #24

Support for nng_msg as Message #24

Conversation

codypiersall commented Jan 12, 2019

goodboy left a comment

Choose a reason for hiding this comment

goodboy Jan 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodboy Jan 13, 2019 • edited Loading

Choose a reason for hiding this comment

codypiersall commented Jan 17, 2019

codypiersall commented Jan 17, 2019

goodboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codypiersall commented Jan 21, 2019

goodboy Jan 13, 2019 •

edited

Loading

goodboy Jan 13, 2019 •

edited

Loading