Request-reply (Client-Server) support #219

eile · 2017-07-18T12:07:17Z

No description provided.

hernando

Only first commit reviewed

hernando · 2017-07-19T13:03:58Z

zeroeq/detail/context.h

+using ContextPtr = std::shared_ptr<void>;
+
+/** @return the current ZeroMQ context. Users need to hold onto this context to
+ *          extend its lifetime to all sockets created from the context.


This documentation is not precise. The context is created if it doesn't exist.

This function is a bit strange for such a simple name as getContext.
Why storing a weak reference to the context internally? It seems as if you wanted to guarantee that all sockets will share the same context implicitly as long there's one object holding a context and remove the context once the last object using it is destroyed. If that's the intention it should be documented somewhere. I would rename it to getOrCreateSharedContext()

hernando · 2017-07-19T13:38:09Z

tests/reqRep.cpp

+    BOOST_CHECK(handled);
+}
+
+#if 0


Isn't there a better way to skip tests that are not ready with Boost.UnitTest? At least In Python there's a decorator to do it.

That's gone now.

hernando · 2017-07-19T13:47:22Z

tests/reqRep.cpp

+    std::thread thread([&] { runOnce(server, echo); });
+
+    bool handled = false;
+    client.get(echo.getTypeIdentifier(), [&](const void* data,


Is there any difference between using echo.getTypeIndenitifier() and test::Echo::IDENTIFIER?

The first can be called from the object without knowing it's type, and the second one is static and can be called without an instance (but needs the type)

hernando · 2017-07-19T13:54:44Z

tests/reqRep.cpp

+    zeroeq::Server noServer(client.getSession());
+    zeroeq::detail::Sender::getUUID() =
+        servus::make_UUID(); // different machine
+    zeroeq::Server server(client.getSession());


Can you explain this a little better. Rename noServer to notConnected or unreachable?

that's gone. I started with a c&p from the pubsub tests

hernando · 2017-07-19T14:02:52Z

tests/reqRep.cpp

+    std::thread thread([&] { runOnce(server, echo); });
+
+    bool handled = false;
+    client.get(echo.getTypeIdentifier(), [&](const void* data,


This is something that was not clearly specified in the feature request. Client get/set methods were mentioned, but only in a example, and the example doesn't match this test. What's the value of this get method over Client::request, because I don't see many differences.

hernando · 2017-07-19T14:04:32Z

tests/reqRep.cpp

+            received = true;
+            break;
+        }
+    }


I'm confused. Isn't this for request/reply tests? Why Client has a method called publish? Why servers has a subscribe method? This doesn't resemble the feature spec at all.

I think I see. You just copied the test from pubSub and "commented" this part. If the partial commits don't make sense it's a waste of time to look into them, but looking at the full pull request at once is really hard because of its size. What should I expect from the rest of the commits and what can I expect to be less time consuming? I was already feeling this PR was going to be hard to review and this commit hasn't improved that feeling.

hernando · 2017-07-19T14:16:00Z

zeroeq/client.h

+ * once the servers are available.
+ *
+ * A receive on any instance of a shared group will work on all instance
+ * and call the registered handlers.


Rephrase this sentence.

hernando · 2017-07-19T14:17:39Z

zeroeq/client.h

+                            size_t size, ReplyFunc& func);
+
+    /** @return the session name that is used for filtering. */
+    ZEROEQ_API const std::string& getSession() const;


Should this method be in Receiver?

Monitor is a session-less receiver.

hernando · 2017-07-19T14:18:30Z

zeroeq/client.h

+                            ReplyFunc& func);
+
+    /**
+     * Request the execution of the given data on a connected Server.


"Request the execution of data", that's a funny sentence

hernando · 2017-07-19T14:19:19Z

zeroeq/client.h

+     * Request the execution of the given data on a connected Server.
+     *
+     * The reply function will be executed during receive(). May block of when
+     * all servers are overloaded or no server is connected.


block of, remove of

hernando · 2017-07-19T14:27:51Z

zeroeq/client.h

+     * Request the execution of the given data on a connected Server.
+     *
+     * The reply function will be executed during receive(). May block of when
+     * all servers are overloaded or no server is connected.


Is it true that this function may block (and same for the other overload)? Isn't receive the function that blocks?
If this function can block, then it should accept a timeout parameter or at least there should be a way to interrupt it.
How does zeromq work with REQ/REP sockets anyway? To me it seems a bit odd to make requests to an inexistent resource. I'm also missing a way to know that the client is connected to any server without having to use application level messages for that. It's also not clear to me whether a user may want to have a client which is not connected to anything after the constructors return.

Yes, it's true since the request sending will block. Receive() will block on at least one reception of a reply.

I would add a timeout on an as-needed basis. One needs to use ZMQ_DONTWAIT and spin, afaik. In reality I expect this to be a non-issue.

One can connect to a zeroconf session which will dynamically update the servers as they are available. The Monitor can be used to get notifications.

eile · 2017-07-20T12:49:36Z

Retest this please.

eile · 2017-07-21T07:25:55Z

Retest this please.

eile · 2017-07-28T08:37:36Z

Retest this please.

eile · 2017-07-28T09:36:31Z

ping?

eile force-pushed the reqrep branch from 4af6cb6 to d31c124 Compare July 18, 2017 12:09

eile assigned tribal-tec, rdumusc and hernando Jul 18, 2017

eile force-pushed the reqrep branch 6 times, most recently from 4ce5de4 to d3a4f84 Compare July 19, 2017 09:57

hernando reviewed Jul 19, 2017

View reviewed changes

eile force-pushed the reqrep branch 2 times, most recently from c6b2358 to 2333a50 Compare July 19, 2017 14:05

hernando reviewed Jul 19, 2017

View reviewed changes

eile force-pushed the reqrep branch 2 times, most recently from 31f92ed to fa62b74 Compare July 19, 2017 14:24

hernando reviewed Jul 19, 2017

View reviewed changes

eile force-pushed the reqrep branch from fa62b74 to 57626eb Compare July 20, 2017 08:20

eile pushed a commit to eile/ZeroEQ that referenced this pull request Jul 20, 2017

CR HBPVIS#219

72365d4

eile force-pushed the reqrep branch from 72365d4 to b64eb3f Compare July 20, 2017 09:45

eile pushed a commit to eile/ZeroEQ that referenced this pull request Jul 20, 2017

CR HBPVIS#219

b64eb3f

eile force-pushed the reqrep branch 2 times, most recently from f41d03d to 8ea075a Compare July 20, 2017 12:29

eile pushed a commit to eile/ZeroEQ that referenced this pull request Jul 21, 2017

CR HBPVIS#219 II

37314a1

eile force-pushed the reqrep branch from 37314a1 to 0fcd9c4 Compare July 24, 2017 08:09

eile pushed a commit to eile/ZeroEQ that referenced this pull request Jul 24, 2017

CR HBPVIS#219

30dd41c

eile pushed a commit to eile/ZeroEQ that referenced this pull request Jul 24, 2017

CR HBPVIS#219 II

0fcd9c4

eile force-pushed the reqrep branch from 0fcd9c4 to 93e0f86 Compare July 24, 2017 08:41

eile pushed a commit to eile/ZeroEQ that referenced this pull request Jul 27, 2017

CR HBPVIS#219 IV

afff06e

Stefan Eilemann added 14 commits July 27, 2017 15:38

Clean up and fix zmq context usage

88b185b

Tests for req-rep

bbe7f8d

Client API

ffd74de

Server API

a630f94

Implementation

beabffd

Allow and test inproc:// URIs

7f687de

Unify naming between ContextPtr and SocketPtr

565827f

req-rep perf test

96ef5c7

CR HBPVIS#219

aa32bfb

Fix exit deadlock due to wrong socket-context dtor order

8ae50dc

CR HBPVIS#219 II

051d7ad

Better zmq_socket_monitor robustness

2eb6341

CR HBPVIS#219 III

cf51469

Improve test coverage

a18c0e1

eile force-pushed the reqrep branch from afff06e to d9169fb Compare July 27, 2017 13:44

eile pushed a commit to eile/ZeroEQ that referenced this pull request Jul 27, 2017

CR HBPVIS#219 IV

d9169fb

eile mentioned this pull request Jul 27, 2017

throw in handle functions not handled #224

Closed

eile force-pushed the reqrep branch from d9169fb to b6e588d Compare July 28, 2017 07:25

eile pushed a commit to eile/ZeroEQ that referenced this pull request Jul 28, 2017

CR HBPVIS#219 IV

b6e588d

eile force-pushed the reqrep branch from b6e588d to 92178a5 Compare July 28, 2017 07:49

Stefan Eilemann added 2 commits July 28, 2017 10:21

Fix spurious test failure in reqRep

014311c

CR HBPVIS#219 IV

0cb23f0

eile force-pushed the reqrep branch from 92178a5 to 0cb23f0 Compare July 28, 2017 08:22

eile mentioned this pull request Jul 28, 2017

Fix #224: Handle exceptions in server handlers #225

Merged

tribal-tec approved these changes Jul 28, 2017

View reviewed changes

eile merged commit 696a15d into HBPVIS:master Jul 28, 2017

eile deleted the reqrep branch July 28, 2017 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request-reply (Client-Server) support #219

Request-reply (Client-Server) support #219

eile commented Jul 18, 2017

hernando left a comment

hernando Jul 19, 2017

hernando Jul 19, 2017

eile Jul 20, 2017

hernando Jul 19, 2017

eile Jul 20, 2017

hernando Jul 19, 2017

eile Jul 20, 2017

hernando Jul 19, 2017

hernando Jul 19, 2017

hernando Jul 19, 2017

eile Jul 20, 2017

hernando Jul 19, 2017

eile Jul 20, 2017

hernando Jul 19, 2017

hernando Jul 19, 2017

eile Jul 20, 2017

hernando Jul 19, 2017

eile Jul 20, 2017

eile Jul 20, 2017

eile Jul 20, 2017

eile commented Jul 20, 2017

eile commented Jul 21, 2017

eile commented Jul 28, 2017

eile commented Jul 28, 2017

Request-reply (Client-Server) support #219

Request-reply (Client-Server) support #219

Conversation

eile commented Jul 18, 2017

hernando 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

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

eile commented Jul 20, 2017

eile commented Jul 21, 2017

eile commented Jul 28, 2017

eile commented Jul 28, 2017