L41: Asynchronous port binding method in Node Server API#108
L41: Asynchronous port binding method in Node Server API#108murgatroid99 merged 4 commits intogrpc:masterfrom
Conversation
cjihrig
left a comment
cjihrig
left a comment
There was a problem hiding this comment.
Makes sense to me. I ran into this exact issue in https://www.npmjs.com/package/grpc-server-js. I made it an async function to make it seem more synchronous (and because older versions of Node aren't supported), but a callback API makes as much sense.
L41-node-server-async-bind.md
Outdated
|
|
||
| ## Background | ||
|
|
||
| In the initial development of the Node gRPC library, the `Server` class method to bind a port, called `bind`, was implemented as a synchronous operation because it was available as an asynchronous operation in the underlying core library. In contrast, in the Node built in `net` module, the `Server` class's `listen` method, which performs a similar function, is asynchronous. In the new pure JavaScript Node gRPC library, the underlying network operations use the `net` module, so it is impossible to implement the `bind` method as a synchronous operation. |
There was a problem hiding this comment.
"it was available as an asynchronous operation" --> "it was available as a synchronous operation"?
L41-node-server-async-bind.md
Outdated
| * @param creds Server credentials object to be used for SSL. | ||
| * @param callback Callback to be called asynchronously with the result of binding the port | ||
| */ | ||
| bindAsync(port: string, creds: ServerCredentials, callback: (port: number)=>void): void; |
There was a problem hiding this comment.
Should the callback take an error as the first argument? That would be more "Node like" and let this method work with things like util.promisify().
There was a problem hiding this comment.
I am on the fence about that. On one hand, you make a good point about util.promisify() and consistency with other Node operations. On the other hand, the current bind API communicates failure with specific return values instead of throwing errors, so passing an error to the callback would be a greater semantic deviation from the existing API.
There was a problem hiding this comment.
It's your call. But if your implementation doesn't end up being compatible with util.promisify() out of the box, you might want to look into custom promisified functions.
There was a problem hiding this comment.
Actually, after thinking about it more I think we can have it both ways, or close enough. If we pass an error to the callback on failure, and also pass the number unconditionally, then users who want to can do the same checks with the number that they do now, and we will have compatibility with util.promisify().
L41-node-server-async-bind.md
Outdated
|
|
||
| ## Implementation | ||
|
|
||
| I (@murgatroid99) will implement this in the grpc library as soon as this proposal is accpeted, and in the @grpc/grpc-js library as part of the server implementation. |
L41-node-server-async-bind.md
Outdated
|
|
||
| ## Rationale | ||
|
|
||
| The sematics of `bindAsync` would be identical to the semantics of `bind` except it is asynchronous, so the name communicates that. Keeping the semantics and API as similar as possible minimizes the work need to transition between the two. We could also implement the asynchrony using promises, but a callback based API is more consistent with the rest of the existing API. In the future, promise support could be added, perhaps by returning a promise when no callback is provided. |
3 participants
Discussion thread: https://groups.google.com/forum/#!topic/grpc-io/Uio1Ja6GRHI