New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streaming RPCs / GRPC Compatibility #529

Closed
paralin opened this Issue Dec 8, 2016 · 23 comments

Comments

Projects
None yet
2 participants
@paralin

paralin commented Dec 8, 2016

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

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

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

This comment has been minimized.

paralin commented Dec 8, 2016

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

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

This comment has been minimized.

paralin commented Dec 8, 2016

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

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

This comment has been minimized.

paralin commented Dec 8, 2016

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

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

This comment has been minimized.

paralin commented Dec 8, 2016

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

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 added a commit that referenced this issue Dec 8, 2016

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

Example

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

dcodeIO added a commit that referenced this issue Dec 8, 2016

dcodeIO added a commit that referenced this issue Dec 8, 2016

@paralin

This comment has been minimized.

paralin commented Dec 8, 2016

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

Seems loadSync will be a thing pretty soon :)

@paralin

This comment has been minimized.

paralin commented Dec 8, 2016

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

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

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

This comment has been minimized.

paralin commented Dec 8, 2016

@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

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

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

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

Using everything after ; as the root?

@paralin

This comment has been minimized.

paralin commented Dec 8, 2016

@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 paralin changed the title from Streaming RPCs to Streaming RPCs / GRPC Compatibility Dec 8, 2016

@paralin

This comment has been minimized.

paralin commented Dec 8, 2016

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

dcodeIO added a commit that referenced this issue Dec 8, 2016

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 8, 2016

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

dcodeIO added a commit that referenced this issue Dec 8, 2016

@paralin

This comment has been minimized.

paralin commented Dec 9, 2016

Neat, will implement and get back to you.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 14, 2016

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

This comment has been minimized.

Owner

dcodeIO commented Dec 15, 2016

On npm now!

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 19, 2016

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

@dcodeIO dcodeIO closed this Dec 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment