grpc / grpc-node

gRPC for Node.js
https://grpc.io
Apache License 2.0
4.45k stars 645 forks source link

Connect to stdin/stdout? #2038

Open entropitor opened 2 years ago

entropitor commented 2 years ago

Is your feature request related to a problem? Please describe.

To be able to build a buildkit frontend, we have to connect to a GRPC server over stdin/stdout

https://github.com/moby/buildkit/blob/914e64243dbaa2ccb8798d732c6f7bafbb708eaa/frontend/gateway/grpcclient/client.go#L390-L392 https://github.com/moby/buildkit/blob/914e64243dbaa2ccb8798d732c6f7bafbb708eaa/frontend/gateway/grpcclient/client.go#L406-L408

However, I don't see how this should work with @grpc/grpc-js. Is there an easy way to pass streams to a Client / Channel so they can be used instead of an url?

Describe the solution you'd like

A way to pass streams / an option to use stdin&stdout for the client as a channel

Describe alternatives you've considered

Extending a Channel and passing this to a client, but there doesn't seem an easy way to pass an input and output stream to

nicolasnoble commented 2 years ago

The fact is that Go has enough abstraction layers to allow for stdio to be used instead of a TCP connection. The problem with that is that this implies a pair of handles to communicate, one in, and one out. Moreover, the @grpc/grpc-js package is using the node runtime itself to decapsulate the http2 layer. The resulting problem here is that while Go allows you to do this, I don't believe there's such mechanism within the nodejs runtime to use stdio instead of a tcp socket within the http2 codec; I might be wrong, but then again, using stdio isn't really a supported feature of the grpc project, you only so happened to have stumbled upon something that the Go runtime permits, and this works only by chance.

aminick commented 1 year ago

I think you can create a custom connection within the credentials

something like this

creds._getConnectionOptions = () => {
  return {
    secureContext: {},
    createConnection: () => {
      return <Custom_Connection>;
    },
  };
};

and i believe any Duplex should work as the connection

robatwilliams commented 5 months ago

The change in https://github.com/grpc/grpc-node/pull/2675 by @murgatroid99 looks like it would allow a stdin/stdout pair wrapped as a stream.Duplex to be used as a connection at the server side. This and the above answer seem to allow any stream to be used instead of a TCP connection.

Could be useful for parent/child process communication using an anonymous pipe.

However I'm not too sure (new area for me) and I haven't tried it.

robatwilliams commented 5 months ago

I've implemented a stdio proof of concept here, using the aforementioned techniques on the client and server side. There is an explanation in the readme. It shows, as far as I can judge, that it is feasible to allow an arbitrary transport duplex stream.

The client side technique however seems fragile - we are tricking the library into not going into the else block in createSession() which would overwrite our createConnection(). Could we have some facility to do this cleanly via the API?

@nicolasnoble - with that, and the two years since your last analysis, what do you think? Thanks

nicolasnoble commented 5 months ago

I'm sorry but I am no longer working on the grpc project at Google.

robatwilliams commented 5 months ago

No worries, thanks.

@murgatroid99 I see you're currently very active here. What do you think?

I've implemented a stdio proof of concept here, using the aforementioned techniques on the client and server side. There is an explanation in the readme. It shows, as far as I can judge, that it is feasible to allow an arbitrary transport duplex stream.

The client side technique however seems fragile - we are tricking the library into not going into the else block in createSession() which would overwrite our createConnection(). Could we have some facility to do this cleanly via the API?

murgatroid99 commented 5 months ago

You linked to this issue instead of your proof of concept.

If you're injecting the connection using credentials as mentioned in the earlier comment, that is an unintended hack that I wouldn't expect to be robust. The proper way would be to add a new public API to do this, similar to the connection injector API on the server, but I think it's not obvious what that should look like.

robatwilliams commented 5 months ago

Apologies, corrected the link now.

Agreed. I'd be happy to think about the API and propose something, if that would be helpful as a first step to move this forward. Also happy to contribute beyond that.

murgatroid99 commented 5 months ago

One option I see would be to introduce a new Channel factory or constructor that takes a stream instead of a target address. This is probably the cleanest way to do it at the level of the Channel API, but it would be a little awkward to use in a Client because you would need to use the channel override option.

Another option is to add a channel argument that accepts the stream object. This would make it easier to use at the Client level because you wouldn't need to construct the Channel separately, but it would make the Channel changes more complicated.

In either case, it might make sense to accept a stream factory function instead of a single stream, the way the http2 API does.

robatwilliams commented 5 months ago

1st:

const stdio = stream.Duplex.from({ writable: child.stdin, readable: child.stdout });
const client = new hello_proto.Greeter(undefined, undefined, {
  channelOverride: Channel.fromStream(stdio, grpc.credentials.createInsecure())
});

As we're doing something custom for the comms channel, I think it's reasonable to expect some extra code around constructing Channel.

2nd:

const stdio = stream.Duplex.from({ writable: child.stdin, readable: child.stdout });
const client = new hello_proto.Greeter(undefined, grpc.credentials.createInsecure(), {
  stream: stdio
});

This would require the ignoring of address when the stream argument is specified to be documented. In the case of using channelOverride, it's already documented, and it's existing behaviour.