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

(low priority) core,netty,interop-testing: stabilize maxInboundMessageSize API #4399

Merged
merged 5 commits into from
Apr 27, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions core/src/main/java/io/grpc/ManagedChannelBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -293,13 +293,12 @@ public T enableFullStreamDecompression() {
* <p>This method is advisory, and implementations may decide to not enforce this. Currently,
* the only known transport to not enforce this is {@code InProcessTransport}.
*
* @param max the maximum number of bytes a single message can be.
* @throws IllegalArgumentException if max is negative.
* @param bytes the maximum number of bytes a single message can be.
* @return this
* @throws IllegalArgumentException if max is negative.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/max/bytes/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* @since 1.1.0
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/2307")
public T maxInboundMessageSize(int max) {
public T maxInboundMessageSize(int bytes) {
// intentional nop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should add in a checkArgument for bytes >= 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return thisT();
}
Expand Down
19 changes: 19 additions & 0 deletions core/src/main/java/io/grpc/ServerBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,25 @@ public T handshakeTimeout(long timeout, TimeUnit unit) {
throw new UnsupportedOperationException();
}

/**
* Sets the maximum message size allowed to be received on the server. If not called,
* defaults to 4 MiB. The default provides protection to servers who haven't considered the
* possibility of receiving large messages while trying to be large enough to not be hit in normal
* usage.
*
* <p>This method is advisory, and implementations may decide to not enforce this. Currently,
* the only known transport to not enforce this is {@code InProcessServer}.
*
* @param bytes the maximum number of bytes a single message can be.
* @return this
* @throws IllegalArgumentException if max is negative.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/max/bytes/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* @throws UnsupportedOperationException if unsupported.
* @since 1.13.0
*/
public T maxInboundMessageSize(int bytes) {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client-side doesn't throw. Since this is advisory, that seems the better approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed and added comment.

}

/**
* Builds a server using the given parameters.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ void start() throws Exception {
server =
NettyServerBuilder.forPort(port)
.sslContext(sslContext)
.maxMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.addService(
ServerInterceptors.intercept(
new TestServiceImpl(executor), TestServiceImpl.interceptors()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ public static void turnOnAutoWindow() {

@Override
protected AbstractServerImplBuilder<?> getServerBuilder() {
return NettyServerBuilder.forPort(0).maxMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE);
return NettyServerBuilder.forPort(0)
.maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ protected AbstractServerImplBuilder<?> getServerBuilder() {
return NettyServerBuilder
.forAddress(new LocalAddress("in-process-1"))
.flowControlWindow(65 * 1024)
.maxMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.channelType(LocalServerChannel.class);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ protected AbstractServerImplBuilder<?> getServerBuilder() {
try {
return NettyServerBuilder.forPort(0)
.flowControlWindow(65 * 1024)
.maxMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.sslContext(GrpcSslContexts
.forServer(TestUtils.loadCert("server1.pem"), TestUtils.loadCert("server1.key"))
.clientAuth(ClientAuth.REQUIRE)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ protected AbstractServerImplBuilder<?> getServerBuilder() {
contextBuilder.ciphers(TestUtils.preferredTestCiphers(), SupportedCipherSuiteFilter.INSTANCE);
return NettyServerBuilder.forPort(0)
.flowControlWindow(65 * 1024)
.maxMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.sslContext(contextBuilder.build());
} catch (IOException ex) {
throw new RuntimeException(ex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public static void registerCompressors() {
@Override
protected AbstractServerImplBuilder<?> getServerBuilder() {
return NettyServerBuilder.forPort(0)
.maxMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE)
.compressorRegistry(compressors)
.decompressorRegistry(decompressors)
.intercept(new ServerInterceptor() {
Expand Down
14 changes: 12 additions & 2 deletions netty/src/main/java/io/grpc/netty/NettyServerBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -259,10 +259,20 @@ public NettyServerBuilder flowControlWindow(int flowControlWindow) {
* defaults to 4 MiB. The default provides protection to services who haven't considered the
* possibility of receiving large messages while trying to be large enough to not be hit in normal
* usage.
*
* @deprecated Call {@link #maxInboundMessageSize} instead. This method will be removed in a
* future release.
*/
@Deprecated
public NettyServerBuilder maxMessageSize(int maxMessageSize) {
checkArgument(maxMessageSize >= 0, "maxMessageSize must be >= 0");
this.maxMessageSize = maxMessageSize;
return maxInboundMessageSize(maxMessageSize);
}

/** {@inheritDoc} */
@Override
public NettyServerBuilder maxInboundMessageSize(int bytes) {
checkArgument(bytes >= 0, "bytes must be >= 0");
this.maxMessageSize = bytes;
return this;
}

Expand Down