Streaming RPCs / GRPC Compatibility

[paralin]

We need to add a API surface for streaming RPCs.

Have a look at how GRPC node does it: http://www.grpc.io/docs/quickstart/node.html

I would recommend mirroring that exactly and then attempting to convert grpc-node to use the protobuf.js pattern.

[dcodeIO]

Once created through Service.create with a RPC implementation, the API actually looks quite similar:

var Greeter = root.lookup("Greeter");
var greeter = Greeter.create(myRpcImpl);

// Now you can use
greeter.sayHello({ name: "you" }, function(err, res) {
  ...
});

The RPC implementation can be quite everything that takes a request and asynchronously returns a response. Signature:

/**
 * RPC implementation passed to {@link Service#create} performing a service request on network level, i.e. by utilizing http requests or websockets.
 * @typedef RPCImpl
 * @function
 * @param {Method} method Reflected method being called
 * @param {Uint8Array} requestData Request data
 * @param {function(?Error, Uint8Array=)} callback Node-style callback called with the error, if any, and the response data
 * @returns {undefined}
 */

[paralin]

Yes, but it needs to be able to take a stream of request or response messages (not sure if you're just comparing the current implementation or saying it doesn't need changing)

…

On Thu, Dec 8, 2016, 6:38 AM Daniel Wirtz ***@***.***> wrote: Once implemented through Service.create <https://github.com/dcodeIO/protobuf.js/blob/master/src/service.js#L159> with a RPC implementation, the API actually looks quite similar: var Greeter = root.lookup("Greeter");var greeter = Greeter.create(myRpcImpl); // Now you can use (rename the method to begin with a lower case letter here?)greeter.SayHello({ name: "you" }, function(err, res) { ... }); The RPC implementation can be quite everything that takes a request and asynchronously returns a response. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#529 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgpZ44sI7GK7Vj8fdM-pY8_cod1422Oks5rF-wpgaJpZM4LHWRP> .

[dcodeIO]

I referred to the API described at http://www.grpc.io/docs/quickstart/node.html - which doesn't seem to cover streams explicitly - but I assumed it is also the API for streaming buffers, which in this case should be possible to be implemented with a rpcImpl that reads from and writes to a stream instead.

I might just be missing something. Do you have another reference or example of the API you propose?

[paralin]

Try this one: http://www.grpc.io/docs/tutorials/basic/node.html

…

On Thu, Dec 8, 2016, 9:10 AM Daniel Wirtz ***@***.***> wrote: I refer to the API described at http://www.grpc.io/docs/quickstart/node.html - which doesn't seem to cover streams explicitly - but I assumed it is also the API for streaming buffers, which in this case should be possible to be implemented with a rpcImpl. I might just be missing something. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#529 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgpZ_-ugaFg51eEFZckb_TLKNrfLXQ0ks5rGA_WgaJpZM4LHWRP> .

[dcodeIO]

Hmm, stream classes. I'd rather try to avoid these as this potentially adds a lot of usually-unused code to the library. I'll probably try to use the current rpcImpl stuff first to implement streaming RPC. Requirements: Multiple requests, common callback, right?

[paralin]

Yes, as well as a number of events (end, error, data, status, meta)

…

On Thu, Dec 8, 2016, 9:39 AM Daniel Wirtz ***@***.***> wrote: Hmm, stream classes. I'd rather try to avoid these as this potentially adds a lot of usually-unused code to the library. I'll probably try to use the current rpcImpl stuff first to implement streaming RPC. Requirements: Multiple requests, common callback, right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#529 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgpZ7VfllO43PsJD2u6HKNXMb7V9Y8Oks5rGBaYgaJpZM4LHWRP> .

[dcodeIO]

What about...

• data: callback(null, someMessage)
• error: callback(someError)
• end: callback(null, null)

Plus, to signal end on the calling site, sending a null message.

[paralin]

How would you send multiple messages from the client side? Is this really better than the grpc way?

…

On Thu, Dec 8, 2016, 9:45 AM Daniel Wirtz ***@***.***> wrote: What about... - data: callback(null, someMessage) - error: callback(someError) - end: callback(null, null) And to signal end, also sending a null message. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#529 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgpZxGUZrIY0BTOVsiWOhtgXcPt2VUwks5rGBf5gaJpZM4LHWRP> .

[dcodeIO]

From an API perspective, using stream classes will most likely always look and feel better. The alternative I am proposing would simply not require dedicated stream classes at all.

Let me prepare an example.

[dcodeIO]

Example

It's now actually using a minimal event emitter to represent the service.

[paralin]

@dcodeIO see grpc/grpc#8991 and grpc/grpc#9004

[dcodeIO]

Seems loadSync will be a thing pretty soon :)

[paralin]

I'm sitting here converting every single one of their tests to use async load...

Would be great if there was loadSync, haha.

Running into one issue right now @dcodeIO. How do we load with an alternate root dir? Before we could pass an object into load to do this.

[dcodeIO]

There is no such option anymore, but there is:

/**
 * Resolves the path of an imported file, relative to the importing origin.
 * This method exists so you can override it with your own logic in case your imports are scattered over multiple directories.
 * @function
 * @param {string} origin The file name of the importing file
 * @param {string} target The file name being imported
 * @returns {string} Resolved path to `target`
 */
RootPrototype.resolvePath = util.resolvePath;

You could, for example, invent your own prefix that marks the root directory. Or test files against regular expressions etc.

[paralin]

@dcodeIO This is a bit clumsy with the grpc single-function load... I guess if people want a different root they will have to build a protobuf.js Root first and then pass it to loadObject in GRPC

[dcodeIO]

What if protobuf.js would support something like...

protobuf.load("file.proto;../otherdir", ...);

Using everything after ; as the root?

[paralin]

@dcodeIO I solved it like this: grpc/grpc@6c07766#diff-06d976047eb70c6a503cf5b260e71feeR215

That solution is a bit clunky but I guess it could work.

I think the most urgent thing to fix is loadSync since grpc downstream doesn't seem to like loading async (breaks their API too much).

I could put my current solution into a loadAsync function in grpc.

[paralin]

OK, I guess I'm blocked here until loadSync is done. Other than that GRPC should be easy to finish upgrading.

[dcodeIO]

There it is. Didn't really test it yet.

[paralin]

Neat, will implement and get back to you.

[dcodeIO]

Note: I just changed util.resolvePath to be util.path.resolve. Iirc, you are using this explicitly where overriding Root#resolvePath (which keeps its name).

Will be 6.2.0

[dcodeIO]

On npm now!

[dcodeIO]

Closing this issue for now. Feel free to reopen it if necessary!