Client/server (req-rep) specification #213

eile · 2017-06-06T09:24:49Z

No description provided.

eile · 2017-06-07T08:22:00Z

See latest diff for a variant of asynchronous requests. The zmq API (req-rep-dealer-router) and doc is "flexible" for this type of use cases. I hope I got it now for the impl.

rdumusc

Some general thoughts:

There is an apparent contradiction between "Certain operations are by necessity synchronous" and "Requirements: Asynchronous remote procedure call".
If the API is asynchronous it must return a future so that users can wait on the request completion.

I see the following use cases (requirements?) to shape the API:

get a remote object, no parameters. This is the most trivial and common use case, such as the livreGUI connecting to a renderer and requesting initial state.
get a remote object from a collection, with parameters passing for "filtering/selection", such as getting a specific block in the livre::RemoteDataSource example here or a group of morphologies from a circuit.
set a remote object directly, i.e. without requiring that the server has "subscribed" to the client.
basic RPC (execute a remote function) with or without return value and with or without parameters. It's easy enough to do basic RPC but it quickly becomes a complicated topic with future chaining, etc, better addressed by frameworks such as https://capnproto.org/rpc.html or HPX.
- 1. are the equivalent of remote getters-setters for a property, and PUB-SUB can form the equivalent of notifyChanged signals.

Relationship with http::Server:

and 2) are like handle(GET) in http::Server. 3) is like the handle(PUT) in http server. 4) doesn't have an exact http equivalent.

eile · 2017-06-07T10:14:25Z

There is an apparent contradiction between "Certain operations are by necessity synchronous" and "Requirements: Asynchronous remote procedure call".

The exchange is synchronous, the API is not. Will clarify.

eile · 2017-06-07T10:16:14Z

If the API is asynchronous it must return a future

No, it can also take a functor which is called on completion. I picked this approach since it does not require the reply object (if any) to be passed into the get().

eile · 2017-06-07T10:16:59Z

I'll pick your use cases into the Examples section.

eile · 2017-06-07T10:20:33Z

On reading them, maybe not: 1, 2 and 3 are special cases of 4:

is 4 with a return value
is 4 with parameters and a return value
is 4 with parameters

eile · 2017-06-07T10:22:51Z

Or, to phrase it differently: The get Serializable is both your function name (the type uuid) and parameters (object data, may be empty), and the ReplyFunc is your return value. With this you can do any remote call.

hernando · 2017-06-07T11:02:40Z

doc/feature/ReqRep.md

+
+    using ReplyFunc = std::function<void(const void*, size_t)>;
+
+    class Client : public Receiver


Is the inheritance from Receiver important from the user's point of view or is it just convenient for the implementation? If it's the latter replace it with implementetion-defined in the spec.

Yes, it is important: The user can call a single receive() on a set of publishers, http::servers, clients and servers.

hernando · 2017-06-07T11:07:15Z

doc/feature/ReqRep.md

+
+## API
+
+    using ReplyFunc = std::function<void(const void*, size_t)>;


Refresh my memory, do Zerobuf objects check the type ID during deserialization? Otherwise we are lacking type safety here, however I don't have a good suggestion to fix it (except add the type check in Zerobuf).
In any case, if the server doesn't use Zerobuf or some other typesafe serialization engine, we are in trouble.

hernando · 2017-06-07T11:10:29Z

doc/feature/ReqRep.md

+
+### 2: Is the client a receiver?
+
+_Resolution: Yes:_ The get method only sends the request, the reply is received in receive() and correlated to the request.


If a client issues millions of requests and doesn't call receive at all, what does it happen on the server?

Implementation-specific. zmq has high-water marks on pending requests, in this case the server reply will block eventually.

But can the server block in a single reply if the client doesn't process it? Or the behaviour in this case is a socket parameter?

It's a socket parameter, and afaik a single reply won't block.

hernando · 2017-06-07T11:11:06Z

doc/feature/ReqRep.md

+    class Client : public Receiver
+    {
+        // ctors same as Subscriber
+        void get(const Serializable& request, ReplyFunc& func);


How are server side errors propagated to the client?

See below: func( nullptr, 0 ) is called.

That's not entirely satisfactory. In a request that has an empty reply (item 3 in the list from Raphael) how can I distinguish a success from a failure?

hernando · 2017-06-07T11:23:34Z

doc/feature/ReqRep.md

+                            memcpy data to block
+                        });
+
+            _client.receive( TIMEOUT );


To me it looks a lot more natural getting a future from _client.get and calling future.get to resolve it. Is there a way to achieve this without falling into thread-safety traps, or there's a fundamental reason for not doing it?

When does the receive happen in your case? The app still needs to call it, i.e., future.wait() would deadlock in a single-threaded application. The user can still wrap it in a future with a lambda function.

I didn't even think about it because that's an implementation detail. I'd argue that mixing request/reply (which are synchronous) with pub/sub receivers or even receivers from different servers doesn't look like a good idea. I don't know about the implementation details from zmq, but isn't the call to receive the reply blocking? Can you still poll multiple request sockets for the reply?

Wrong thread, I guess. It is not an implementation detail. Client::request() is async, and Client::receive() is the sync point. In the sync point the app handles the reply from the server, which is symmetric with the subscription handling.

I don't have a strong opinion here. I was just wondering if it could be more natural with the future as Raphael suggested. Right now it looks quite symmetric with the pub, sub, but error handling still looks clunky this way. Callback oriented APIs normally have hooks also for errors.

hernando · 2017-06-07T11:27:37Z

doc/feature/ReqRep.md

+        {
+            ConstURIPtr uri = livre::URI::create(data, size);
+            if( openAndCacheDatasource( *uri ))
+                return dataSource.getVolumeInfo().toBinary();


The binary buffer returned will be moved or copied?

shared, Serializable::Data has a shared_ptr + size.

hernando · 2017-06-07T11:28:33Z

doc/feature/ReqRep.md

+routing to a function. This requires that the client and server use the same
+object and version for this, i.e., the hard versioning of ZeroBuf wrt member
+layout is lost. The ID is not part of the ZeroBuf, so would need separate
+transport and checking in server/client.


I see, this "answers" my first comment.

The first comment from my POV does not make sense. Which one do you mean?

BTW: Would be happy to get KISS ideas on how to resolve this.

My first comment is the first comment from my review, which even from your point of view is unique because it's at the very top.

I guess it's the one at the very bottom!?:

Using a future and a template...

See discussion there.

hernando · 2017-06-07T11:29:47Z

tests/common.h

@@ -1,5 +1,5 @@

-/* Copyright (c) 2014-2016, Human Brain Project
+/* Copyright (c) 2014-2017, Human Brain Project


Good that you renamed it, I never liked the name if this file.

hernando · 2017-06-07T11:36:27Z

Why not call get, request? That name doesn't imply there's a return value at all.

Using a future and a template function would allow to introduce some type safety at least in the API. Even if type-safety is not implemented initially, it could be implemented later. The future also provides a natural way of propagating exceptions from the server and allows proper handling in the client. Right now it's not clear at all how errors are handled.

eile · 2017-06-07T11:46:07Z

Why not call get, request? That name doesn't imply there's a return value at all.

Good idea. Will change.

eile · 2017-06-07T11:48:02Z

future and a template function would allow to introduce some type safety

Some, but not all. The bigger issue is that the reply object changes memory layout and we get a client-server mismatch. Since it would still be the same class, a future would not catch (ha!) this. The reply type id needs to be part of the protocol.

hernando · 2017-06-07T12:20:00Z

The future forces the client to tell the implementation which is the expected type, the implementation still needs some mechanism to gurantee that (which at the moment is missing). But since it also depends on changing the protocol, it's not so different from the current proposal in terms of guarantees anyway.

eile · 2017-06-07T13:09:14Z

The future forces the client to tell the implementation which is the expected type, the implementation still needs some mechanism to gurantee that (which at the moment is missing). But since it also depends on changing the protocol, it's not so different from the current proposal in terms of guarantees anyway.

The implementation should not care about the type, but about the type id which is a stronger guarantee and the only one used in other parts of the API. One gains exactly nothing with the template method, because even when using the concrete type it does not prevent sending another type from the server. Note that the sync API in the first iteration used get(Serializable& req, Serializable& rep).

eile · 2017-06-08T12:44:00Z

reply typing addressed in a1193a7

eile · 2017-06-08T14:36:17Z

See https://github.com/eile/ZeroEQ/tree/reqrep for WIP

eile · 2017-06-19T12:41:37Z

Ping: What's left to discuss here?

tribal-tec · 2017-06-20T08:40:14Z

zeroeq/subscriber.h

-     * @throw std::runtime_error if ZeroConf is not available or if session name
-     *                           is invalid
-     */
-    ZEROEQ_API Subscriber(const URI& uri, const std::string& session);


Needs version bump. we should already go to the version 1.0.0 now I guess.

done, to 0.9 for now. Also squashed commits in preparation of rebase-and-merge

eile · 2017-06-23T14:45:09Z

I'll eventually need an approval...

rdumusc

Good for now, maybe we will reevaluate some details after having had some real world experience with the API

eile assigned tribal-tec, hernando and rdumusc Jun 6, 2017

eile force-pushed the master branch 2 times, most recently from 31d680f to 199bfc2 Compare June 6, 2017 12:37

rdumusc reviewed Jun 7, 2017

View reviewed changes

eile force-pushed the master branch from 3a03838 to e63be7d Compare June 7, 2017 11:16

hernando reviewed Jun 7, 2017

View reviewed changes

eile force-pushed the master branch from e63be7d to 3b315aa Compare June 7, 2017 12:59

Cleanups

48b98d1

eile force-pushed the master branch from 98a607e to a1193a7 Compare June 8, 2017 12:43

eile force-pushed the master branch from f5aad33 to 9f9c239 Compare June 19, 2017 12:40

tribal-tec reviewed Jun 20, 2017

View reviewed changes

Cleanup: remove nonsensical Subscriber(uri, session) ctors

2cfceb5

eile force-pushed the master branch from 9f9c239 to 89d8a1f Compare June 20, 2017 11:45

rdumusc approved these changes Jun 23, 2017

View reviewed changes

Client/server (req-rep) specification

0ef7e13

eile force-pushed the master branch from 89d8a1f to 0ef7e13 Compare June 29, 2017 08:49

eile merged commit 40e042a into HBPVIS:master Jun 29, 2017


		using ReplyFunc = std::function<void(const void*, size_t)>;

		class Client : public Receiver


		## API

		using ReplyFunc = std::function<void(const void*, size_t)>;


		### 2: Is the client a receiver?

		_Resolution: Yes:_ The get method only sends the request, the reply is received in receive() and correlated to the request.

		@@ -1,5 +1,5 @@

		/* Copyright (c) 2014-2016, Human Brain Project
		/* Copyright (c) 2014-2017, Human Brain Project

Client/server (req-rep) specification #213

Client/server (req-rep) specification #213

Conversation

eile commented Jun 6, 2017

eile commented Jun 7, 2017

rdumusc left a comment

Choose a reason for hiding this comment

eile commented Jun 7, 2017

eile commented Jun 7, 2017

eile commented Jun 7, 2017

eile commented Jun 7, 2017

eile commented Jun 7, 2017 • 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

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 Jun 7, 2017 • 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

hernando commented Jun 7, 2017

eile commented Jun 7, 2017

eile commented Jun 7, 2017

hernando commented Jun 7, 2017

eile commented Jun 7, 2017

eile commented Jun 8, 2017

eile commented Jun 8, 2017

eile commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eile commented Jun 23, 2017

rdumusc left a comment

Choose a reason for hiding this comment

eile commented Jun 7, 2017 •

edited

Loading

eile Jun 7, 2017 •

edited

Loading