- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3.9k
11246 :: Unexpected error when server expands a compressed message to learn it is too large #12360
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
base: master
Are you sure you want to change the base?
Changes from all commits
fe36e6d
              61fbe27
              bb929aa
              1024a7f
              5410196
              14adfdb
              631edd3
              92b75a8
              894dc66
              65fbebb
              3b9ebbe
              55741e4
              26095ef
              f16cd6b
              a511a77
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -327,16 +327,20 @@ private void messagesAvailableInternal(final MessageProducer producer) { | |
| return; | ||
| } | ||
|  | ||
| InputStream message; | ||
| try { | ||
| InputStream message; | ||
| while ((message = producer.next()) != null) { | ||
| try { | ||
| listener.onMessage(call.method.parseRequest(message)); | ||
| } catch (Throwable t) { | ||
| ReqT parsedMessage; | ||
| try (InputStream ignored = message) { | ||
| parsedMessage = call.method.parseRequest(message); | ||
| } catch (StatusRuntimeException e) { | ||
| GrpcUtil.closeQuietly(message); | ||
| throw t; | ||
| GrpcUtil.closeQuietly(producer); | ||
| call.cancelled = true; | ||
| call.close(e.getStatus(), new Metadata()); | ||
| return; | ||
| } | ||
| message.close(); | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. before  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. while ((message = producer.next()) != null) {
  ReqT parsedMessage;
  try (InputStream ignored = message) {
    parsedMessage = call.method.parseRequest(message);
  } catch (StatusRuntimeException e) {
    GrpcUtil.closeQuietly(producer);
    call.cancelled = true;
    call.close(e.getStatus(), new Metadata());
    return;
  }
  listener.onMessage(parsedMessage);
}There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @panchenko ,Thank you for clarifying the suggestions. I apologize for the misunderstanding; I initially assumed I should remove listener.onMessage(parsedMessage); along with the try and adding the catch to previous try , which caused failures in existing test cases. I have now corrected this and addressed the comments." | ||
| listener.onMessage(parsedMessage); | ||
| } | ||
| } catch (Throwable t) { | ||
| GrpcUtil.closeQuietly(producer); | ||
|  | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -17,6 +17,7 @@ | |
| package io.grpc.testing.integration; | ||
|  | ||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertThrows; | ||
| import static org.junit.Assert.assertTrue; | ||
|  | ||
| import com.google.protobuf.ByteString; | ||
|  | @@ -37,6 +38,8 @@ | |
| import io.grpc.ServerCall.Listener; | ||
| import io.grpc.ServerCallHandler; | ||
| import io.grpc.ServerInterceptor; | ||
| import io.grpc.Status.Code; | ||
| import io.grpc.StatusRuntimeException; | ||
| import io.grpc.internal.GrpcUtil; | ||
| import io.grpc.netty.InternalNettyChannelBuilder; | ||
| import io.grpc.netty.InternalNettyServerBuilder; | ||
|  | @@ -53,7 +56,9 @@ | |
| import java.io.OutputStream; | ||
| import org.junit.Before; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Rule; | ||
| import org.junit.Test; | ||
| import org.junit.rules.TestName; | ||
| import org.junit.runner.RunWith; | ||
| import org.junit.runners.JUnit4; | ||
|  | ||
|  | @@ -84,10 +89,16 @@ public static void registerCompressors() { | |
| compressors.register(Codec.Identity.NONE); | ||
| } | ||
|  | ||
| @Rule | ||
| public final TestName currentTest = new TestName(); | ||
|  | ||
| @Override | ||
| protected ServerBuilder<?> getServerBuilder() { | ||
| NettyServerBuilder builder = NettyServerBuilder.forPort(0, InsecureServerCredentials.create()) | ||
| .maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE) | ||
| .maxInboundMessageSize( | ||
| DECOMPRESSED_MESSAGE_TOO_LONG_METHOD_NAME.equals(currentTest.getMethodName()) | ||
| ? 1000 | ||
| : AbstractInteropTest.MAX_MESSAGE_SIZE) | ||
| .compressorRegistry(compressors) | ||
| .decompressorRegistry(decompressors) | ||
| .intercept(new ServerInterceptor() { | ||
|  | @@ -126,6 +137,22 @@ public void compresses() { | |
| assertTrue(FZIPPER.anyWritten); | ||
| } | ||
|  | ||
| private static final String DECOMPRESSED_MESSAGE_TOO_LONG_METHOD_NAME = | ||
| "decompressedMessageTooLong"; | ||
|  | ||
| @Test | ||
| public void decompressedMessageTooLong() { | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This e2e test is great but the unit test you previously wrote in ServerCallImplTest also lets us test code paths in a more fine-grained way. In particular lets re-introduce that test and add the following after causing the parse exception to happen in message handling - in order to test that  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @kannanjgithub for the review and suggestions. I've addressed the comments , please review the changes when you have a moment. | ||
| assertEquals(DECOMPRESSED_MESSAGE_TOO_LONG_METHOD_NAME, currentTest.getMethodName()); | ||
| final SimpleRequest bigRequest = SimpleRequest.newBuilder() | ||
| .setPayload(Payload.newBuilder().setBody(ByteString.copyFrom(new byte[10_000]))) | ||
| .build(); | ||
| StatusRuntimeException e = assertThrows(StatusRuntimeException.class, | ||
| () -> blockingStub.withCompression("gzip").unaryCall(bigRequest)); | ||
| assertCodeEquals(Code.RESOURCE_EXHAUSTED, e.getStatus()); | ||
| assertEquals("Decompressed gRPC message exceeds maximum size 1000", | ||
| e.getStatus().getDescription()); | ||
| } | ||
|  | ||
| @Override | ||
| protected NettyChannelBuilder createChannelBuilder() { | ||
| NettyChannelBuilder builder = NettyChannelBuilder.forAddress(getListenAddress()) | ||
|  | ||
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.
additionally need
GrpcUtil.closeQuietly(producer);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.
Noted.