Skip to content
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

Connect to stdin/stdout? #2038

Open
entropitor opened this issue Feb 6, 2022 · 10 comments
Open

Connect to stdin/stdout? #2038

entropitor opened this issue Feb 6, 2022 · 10 comments

Comments

@entropitor
Copy link

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
Copy link
Member

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
Copy link

aminick commented Mar 9, 2023

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
Copy link

The change in #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
Copy link

robatwilliams commented May 1, 2024

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
Copy link
Member

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

@robatwilliams
Copy link

robatwilliams commented May 1, 2024

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
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link

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.

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

No branches or pull requests

5 participants