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

Allow consumer of jsapi to provide a custom gRPC transport #6404

Closed
bmingles opened this issue Nov 20, 2024 · 2 comments · Fixed by #6476
Closed

Allow consumer of jsapi to provide a custom gRPC transport #6404

bmingles opened this issue Nov 20, 2024 · 2 comments · Fixed by #6476
Assignees
Labels
Milestone

Comments

@bmingles
Copy link
Contributor

In order to support the vscode extension, we are planning to implement a custom gRPC transport backed by node:http2. We'll need a way to provide this to the Jsapi.

@niloc132
Copy link
Member

We made some good progress here by allowing an instance of improbable-eng/grpc-web's grpc.TransportFactory to be passed in the dh.CoreClient connect options ctor arg - a simple implementation of this that used node:http2 seems to work in both plain node and in vscode itself. This has several advantages over making fetch work:

  • It can support bidirectional h2 streams, unlike fetch in browsers (and presumably that same limitation would be exposed in vscode, if it worked at all in vscode...)
  • It can support h2c connections, removing the need to have a websocket polyfill at all.

It might even be possible that we don't need grpc-web at all for vscode, that we can handle proper trailers and such in here, assign the user-agent header - while that would lower a bit of overhead on the server, it probably isn't worth it for the confusion of trying to tell where the client assumes grpc-web and where it can just rely on the baked in implementation.

@niloc132
Copy link
Member

Proposed API for this:

export namespace dh.grpc {

	/**
	* Options for creating a gRPC stream transport instance.
	*/
	export interface GrpcTransportOptions {
		/**
		* The gRPC method URL.
		*/
		url:URL;
		/**
		* True to enable debug logging for this stream.
		*/
		debug:boolean;
		/**
		* Callback for when headers and status are received. The headers are a map of header names to values, and the
		* status is the HTTP status code. If the connection could not be made, the status should be 0.
		*/
		onHeaders:(headers:{ [key: string]: Array<string>; },status:number)=>void;
		/**
		* Callback for when a chunk of data is received.
		*/
		onChunk:(chunk:Uint8Array,finished:boolean)=>void;
		/**
		* Callback for when the stream ends, with an error instance if it can be provided. Note that the present
		* implementation does not consume errors, even if provided.
		*/
		onEnd:(error:Error|undefined|null)=>void;
	}
	/**
	* gRPC transport implementation.
	*/
	export interface GrpcTransport {
		/**
		* Starts the stream, sending metadata to the server.
		* @param metadata - the headers to send the server when opening the connection
		*/
		start(metadata:{ [key: string]: Array<string>; }):void;
		/**
		* Sends a message to the server.
		* @param msgBytes - bytes to send to the server
		*/
		sendMessage(msgBytes:Uint8Array):void;
		/**
		* "Half close" the stream, signaling to the server that no more messages will be sent, but that the client is still
		* open to receiving messages.
		*/
		finishSend():void;
		/**
		* End the stream, both notifying the server that no more messages will be sent nor received, and preventing the
		* client from receiving any more events.
		*/
		cancel():void;
	}
	/**
	* Factory for creating gRPC transports.
	*/
	export interface GrpcTransportFactory {
		/**
		* Create a new transport instance.
		* @param options - options for creating the transport
		* @return a transport instance to use for gRPC communication
		*/
		create(options:GrpcTransportOptions):GrpcTransport;
		/**
		* Return true to signal that created transports may have {@link GrpcTransport.sendMessage} called on it
		* more than once before {@link GrpcTransport.finishSend} should be called.
		* @return true to signal that the implementation can stream multiple messages, false otherwise indicating that
		*         Open/Next gRPC calls should be used
		*/
		get supportsClientStreaming():boolean;
	}
}

Accessible from a new field on ConnectOptions:

		/**
		* The transport factory to use for creating gRPC streams. If specified, the JS API will ignore
		* {@link dh.useWebsockets} and its own internal logic for determining the appropriate transport to use.
		* <p>
		* Defaults to null, indicating that the JS API should determine the appropriate transport to use. If
		* `useWebsockets` is set to true, the JS API will use websockets, otherwise if the server url begins with
		* https, it will use fetch, otherwise it will use websockets.
		*/
		transportFactory?:dh.grpc.GrpcTransportFactory|null;

niloc132 added a commit to niloc132/deephaven-core that referenced this issue Dec 10, 2024
Provides a contract for client applications to use a custom http/2
implementation. Roughly abstracted from our TypeScript grpc-web library,
with a few rough edges taken off, and no external dependencies.

Two integration tests are included, one which requires https (presently
only possible with manual testing, see deephaven#6421), and one which pretends to
contact the server but really responds to every request with a "success"
response and no payload.

No documentation required at this time, generated typescript includes
details on the new APIs.

Fixes deephaven#6404
devinrsmith pushed a commit that referenced this issue Dec 12, 2024
Provides a contract for client applications to use a custom http/2
implementation. Roughly abstracted from our TypeScript grpc-web library,
with a few rough edges taken off, and no external dependencies.

Two integration tests are included, one which requires https (presently
only possible with manual testing, see #6421), and one which pretends to
contact the server but really responds to every request with a "success"
response and no payload.

No documentation required at this time, generated typescript includes
details on the new APIs.

Fixes #6404
Backport #6476
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants