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

[NodeJS] Promises and async/await #54

Open
alexkreidler opened this Issue Sep 29, 2017 · 8 comments

Comments

Projects
None yet
9 participants
@alexkreidler
Copy link

alexkreidler commented Sep 29, 2017

It would be nice if there was a grpc-tools and/or grpc_node_plugin option that would generate client and server code that follows the ES6 Promise and ES7 async/await model.

For example, instead of:
https://github.com/grpc/grpc/blob/98f8989a6712118549c432c9f91b6a516250014d/examples/node/static_codegen/greeter_client.js#L35-L37
We could write:

  [err, response] = await client.sayHello(request);

Or something similar. On the server side, instead of:
https://github.com/grpc/grpc/blob/98f8989a6712118549c432c9f91b6a516250014d/examples/node/static_codegen/greeter_server.js#L27-L31
We could write:

async function sayHello(call) {
  var reply = new messages.HelloReply();
  reply.setMessage('Hello ' + call.request.getName());
  return [null, reply]
}

Again, just ideas.

This feature would eliminate the need for wrapper code for users wanting to use this pattern. I would love to know if anyone is interested in this feature or planning on adding it. Thanks!

(moved from grpc/grpc#12751)

@murgatroid99

This comment has been minimized.

Copy link
Member

murgatroid99 commented Sep 29, 2017

Thank you for moving this. I think this is an interesting idea that is worth exploring, but there are also some complications you haven't addressed.

On the client side, these functions already have return values, which would be shadowed by a naive implementation of this change, or would need to be merged with the promise object. In fact, it is already possible to get this basic functionality and shadow the return value by using util.promisify in the standard library. I think that merging the Promise objects with the existing return values would work fine, but there's a potential of creating some confusion when part of the object is promise based, and part is not. The main issue that impacts both unary and client streaming calls is that the return object is an event emitter that emits metadata events. So, to get both the response and the metadata from a call, using the async system you propose, the code would have to look something like this:

let call = client.sayHello(request);
call.on('metadata', metadata => {
  // Handle metadata
});
response = await call;  // An error would be thrown, not returned

This isn't so bad, because simply ignoring the metadata and awaiting the return value directly would still work. However, client streaming calls would have the additional issue that you have to write messages and end writing before you can get the response. that code would look more like this:

let call = client.clientStreamingCall();
call.write(message1);
call.write(message2);
call.end();
response = await call;

In this case, directly awaiting the return value would not work, and the function would actually never continue, because the client generally needs to call call.end() before they can expect to receive a response.

On the server side, promise-based unary handlers should be just fine, but for streaming handlers, they should expect to be calling call.on('data'...) and call.on('end',...) to handle the incoming message stream. Still, they can explicitly create and resolve the promise object, so that would not be too bad.

So, I think there's some work to be done here, but if you (or anyone else) submits a fleshed-out proposal for this API addition to https://github.com/grpc/proposal, I would be happy to add that change.

@bojand

This comment has been minimized.

Copy link

bojand commented Oct 4, 2017

Hello, Just wanted to add to the discussion. While I agree that this would be useful and nice, I think it should be approached carefully so we can get it "right" and in line with however Node core approaches async API's as that would probably be the more future-proof and idiomatic mechanism. I would still think for the time being that we have callback based API by default, at least while Node core is still callback-based by default. Expose async API to users explicitly if they ask for it. For example see this experimental proposal for promisified fs. And user's can promisify however they want / need. For example I have created a helper client library that promisifies calls if callbacks are not provided. On server side similarly I've created a minimalistic framework with API that is fully async based and very much inspired by Koa. PR's are welcome for improvements for both.

@marr

This comment has been minimized.

Copy link

marr commented Jan 18, 2018

@bojand I was excited to try out grpc-caller as it seemed to solve the async/await usage with grpc in node. However I came across bojand/grpc-inspect#10 which failed to load my protos containing nested packages. Hopefully there is some way to fix grpc-inspect to resolve the nested packages correctly.

@AlexeySemigradsky

This comment has been minimized.

Copy link

AlexeySemigradsky commented May 10, 2018

The dart version is Future based by default. It will be awesome if the node.js version will also support promises.

@majelbstoat

This comment has been minimized.

Copy link

majelbstoat commented Jun 26, 2018

How about making them observables using something like RxJS @murgatroid99? I'm not an expert on them, but I believe they would support streaming and metadata a little more cleanly, and would be promise-compatible. (https://medium.com/@benlesh/rxjs-observable-interop-with-promises-and-async-await-bebb05306875)

https://github.com/kondi/rxjs-grpc has an implementation of this kind of thing.

(I currently do promisification on the client with bluebird, but because i'm using typescript, i can't use promisifyAll, and have to promisify each method individually 😕)

@aguynamedben

This comment has been minimized.

Copy link

aguynamedben commented Jul 16, 2018

I agree this is an important feature. I'm using https://github.com/carlessistare/grpc-promise#readme and it works as expected so far. I'm not a hardcore gRPC user, however, so I'm not sure what kind of "here be dragons" issues may pop up, but seems worth it for now!

@Jokero

This comment has been minimized.

Copy link

Jokero commented Jul 28, 2018

Promise is already must-have feature. For me callbacks are something from dawn of the dinosaurs :)

@xizhibei

This comment has been minimized.

Copy link

xizhibei commented Sep 9, 2018

Hey guys, I have just created the grpc-helper, a wrapper for the client side.

The promise API:

Unary call:

const message = await helper.SayHello({
  name: 'foo',
});

// withFullResponse: true
const { message, peer, status, metadata } = await helper.SayHello({
  name: 'foo',
});

Client stream call:

const stream = new stream.PassThrough({ objectMode: true });

const promise = helper.SayMultiHello(stream);

stream.write({name: 'foo1'});
stream.write({name: 'foo2'});
stream.write({name: 'foo3'});

stream.end();

// resolveFullResponse: true
const { message, peer, status, metadata } = await promise;
// { message: 'hello foo1,foo2,foo3' }

Besides, I add the features like dns service discovery, load balance and circuit break etc.

Currently in beta, you might as well give it a try, comments and PRs are welcome.

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