-
Notifications
You must be signed in to change notification settings - Fork 78
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
Provide a servlet filter to automatically translate grpc-web clients calls to grpc #2717
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to approve, one thought though.
payload.put(entry.getValue().getBytes(StandardCharsets.US_ASCII)); | ||
payload.put("\r\n".getBytes(StandardCharsets.US_ASCII)); | ||
} | ||
wrappedResponse.getOutputStream().write(payload.array()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth checking that the payload has been fully used (in the case of overflow, we should already get exception)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate, write doesn't take a ByteBuffer, just a byte[]
(or individual byte
s/etc), so presently I'm just writing the whole array in one go (since the size is known, just using ByteBuffer as a convenient way to append bytes as we go), so we must assume that OutputStream.write
will throw if it doesn't write enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, check BB remaining is 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that seems reasonable yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the exception that this update will cause, while still doing its best to not break the socket (though it will omit the trailers, as they have null bytes):
2022-08-29T21:13:42.749Z | qtp417875774-39 | ERROR | i.g.i.SerializingExecutor | Exception while executing runnable io.grpc.servlet.jakarta.AsyncServletOutputStreamWriter$$Lambda$719/0x0000000800678440@6b16a7fd
java.lang.IllegalStateException: null
at io.grpc.servlet.jakarta.web.GrpcWebFilter$1$1.complete(GrpcWebFilter.java:83)
at io.grpc.servlet.jakarta.AsyncServletOutputStreamWriter.lambda$new$1(AsyncServletOutputStreamWriter.java:97)
at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:133)
at com.google.common.util.concurrent.DirectExecutor.execute(DirectExecutor.java:31)
at io.grpc.internal.SerializingExecutor.schedule(SerializingExecutor.java:102)
at io.grpc.internal.SerializingExecutor.execute(SerializingExecutor.java:95)
at io.grpc.servlet.jakarta.ServletServerStream$ServletTransportState.runOnTransportThread(ServletServerStream.java:144)
at io.grpc.servlet.jakarta.AsyncServletOutputStreamWriter.lambda$new$2(AsyncServletOutputStreamWriter.java:94)
at io.grpc.servlet.jakarta.AsyncServletOutputStreamWriter.onWritePossible(AsyncServletOutputStreamWriter.java:143)
at io.grpc.servlet.jakarta.ServletServerStream$GrpcWriteListener.onWritePossible(ServletServerStream.java:218)
at org.eclipse.jetty.server.HttpOutput.run(HttpOutput.java:1493)
at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:1460)
at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:606)
at org.eclipse.jetty.server.HttpChannel.run(HttpChannel.java:457)
at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:412)
at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:381)
at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:268)
at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:190)
at org.eclipse.jetty.http2.HTTP2Connection.produce(HTTP2Connection.java:208)
at org.eclipse.jetty.http2.HTTP2Connection.onFillable(HTTP2Connection.java:155)
at org.eclipse.jetty.http2.HTTP2Connection$FillableCallback.succeeded(HTTP2Connection.java:378)
at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100)
at org.eclipse.jetty.io.ssl.SslConnection$DecryptedEndPoint.onFillable(SslConnection.java:530)
at org.eclipse.jetty.io.ssl.SslConnection.onFillable(SslConnection.java:379)
at org.eclipse.jetty.io.ssl.SslConnection$2.succeeded(SslConnection.java:146)
at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100)
at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:412)
at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:381)
at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:268)
at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.lambda$new$0(AdaptiveExecutionStrategy.java:138)
at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:407)
at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:894)
at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1038)
at java.base/java.lang.Thread.run(Thread.java:829)
Tested by negating the new hasRemaining() check, and confirming that the browser no longer thinks more data is on its way, fails with the truncated payload it did get, and the server log contains the stack trace.
6244e86
to
d291b9c
Compare
d291b9c
to
d87902c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I think we should make sure the commit message has a link to the jetty project issue.
Combined with #2401, the Jetty-based server can now directly offer grpc-web (albeit without support for text payloads). While technically this works with http/1.1 connections, it is best suited for h/2, so the JS API still defaults to the existing websocket transport when https is not present.
See https://github.com/grpc/grpc/blob/v1.48.0/doc/PROTOCOL-WEB.md
Also includes a workaround for jetty/jetty.project#8405.
Fixes #1769