Skip to content

Commit

Permalink
netty: default grace time for RPCs can leak memory
Browse files Browse the repository at this point in the history
Using Long.MAX_VALUE as the delay for Netty's NioEventLoop#schedule can
cause (long) deadlines for scheduled tasks to wrap into negative values,
which is unspecified behavior. Recent versions of netty are guarding
against overflows, but not all versions of grpc-java are using a recent
enough netty.

When connections are gracefully closed, Netty's Http2ConnectionHandler
sets up a timeout task forcing resources to be freed after a grace
period. When their deadlines wrap into negative values, a race was
observed in production systems (with grpc-java <= 1.12.1) causing Netty
to never properly release buffers associated with active streams in the
connection being (gracefully) closed.
  • Loading branch information
fabiokung authored and carl-mastrangelo committed Mar 18, 2019
1 parent e0477bb commit 7df2d5f
Showing 1 changed file with 14 additions and 5 deletions.
19 changes: 14 additions & 5 deletions netty/src/main/java/io/grpc/netty/NettyServerHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static io.grpc.internal.GrpcUtil.SERVER_KEEPALIVE_TIME_NANOS_DISABLED;
import static io.grpc.netty.NettyServerBuilder.MAX_CONNECTION_AGE_GRACE_NANOS_INFINITE;
import static io.grpc.netty.NettyServerBuilder.MAX_CONNECTION_AGE_NANOS_DISABLED;
import static io.grpc.netty.NettyServerBuilder.MAX_CONNECTION_IDLE_NANOS_DISABLED;
import static io.grpc.netty.Utils.CONTENT_TYPE_HEADER;
Expand Down Expand Up @@ -910,19 +911,27 @@ void secondGoAwayAndClose(ChannelHandlerContext ctx) {

// gracefully shutdown with specified grace time
long savedGracefulShutdownTimeMillis = gracefulShutdownTimeoutMillis();
long gracefulShutdownTimeoutMillis = savedGracefulShutdownTimeMillis;
if (graceTimeInNanos != null) {
gracefulShutdownTimeoutMillis = TimeUnit.NANOSECONDS.toMillis(graceTimeInNanos);
}
long overriddenGraceTime = graceTimeOverrideMillis(savedGracefulShutdownTimeMillis);
try {
gracefulShutdownTimeoutMillis(gracefulShutdownTimeoutMillis);
gracefulShutdownTimeoutMillis(overriddenGraceTime);
NettyServerHandler.super.close(ctx, ctx.newPromise());
} catch (Exception e) {
onError(ctx, /* outbound= */ true, e);
} finally {
gracefulShutdownTimeoutMillis(savedGracefulShutdownTimeMillis);
}
}

private long graceTimeOverrideMillis(long originalMillis) {
if (graceTimeInNanos == null) {
return originalMillis;
}
if (graceTimeInNanos == MAX_CONNECTION_AGE_GRACE_NANOS_INFINITE) {
// netty treats -1 as "no timeout"
return -1L;
}
return TimeUnit.NANOSECONDS.toMillis(graceTimeInNanos);
}
}

// Use a frame writer so that we know when frames are through flow control and actually being
Expand Down

0 comments on commit 7df2d5f

Please sign in to comment.