From fb4a7c8233816f79850da18ddc348213938b8588 Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Wed, 18 Mar 2026 10:27:39 -0700 Subject: [PATCH] Fix OneTimeReadTimeoutHandler removed by TLS handshake for 100-continue requests --- .../bugfix-NettyNIOHTTPClient-b32e774.json | 6 + .../netty/internal/NettyRequestExecutor.java | 9 +- .../Expect100ContinueReadTimeoutTest.java | 220 ++++++++++++++++++ 3 files changed, 232 insertions(+), 3 deletions(-) create mode 100644 .changes/next-release/bugfix-NettyNIOHTTPClient-b32e774.json create mode 100644 http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/fault/Expect100ContinueReadTimeoutTest.java diff --git a/.changes/next-release/bugfix-NettyNIOHTTPClient-b32e774.json b/.changes/next-release/bugfix-NettyNIOHTTPClient-b32e774.json new file mode 100644 index 000000000000..4fb7ab39c718 --- /dev/null +++ b/.changes/next-release/bugfix-NettyNIOHTTPClient-b32e774.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "Netty NIO HTTP Client", + "contributor": "", + "description": "Fixed an issue where requests with `Expect: 100-continue` over TLS could hang indefinitely when no response is received, because the read timeout handler was prematurely removed by TLS handshake data." +} diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java index ac862b47d657..c49124ec6da6 100644 --- a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java +++ b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java @@ -265,10 +265,13 @@ private void writeRequest(HttpRequest request) { if (shouldExplicitlyTriggerRead()) { - // Should only add an one-time ReadTimeoutHandler to 100 Continue request. if (is100ContinueExpected()) { - channel.pipeline().addFirst(new OneTimeReadTimeoutHandler(Duration.ofMillis(context.configuration() - .readTimeoutMillis()))); + // Should only add a one-time ReadTimeoutHandler to 100 Continue request. + // Add before HttpStreamsClientHandler so that raw TLS handshake bytes cannot + // prematurely remove this one-time handler. See Expect100ContinueReadTimeoutTest. + channel.pipeline().addBefore( + channel.pipeline().context(HttpStreamsClientHandler.class).name(), null, + new OneTimeReadTimeoutHandler(Duration.ofMillis(context.configuration().readTimeoutMillis()))); } else { channel.pipeline().addFirst(new ReadTimeoutHandler(context.configuration().readTimeoutMillis(), TimeUnit.MILLISECONDS)); diff --git a/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/fault/Expect100ContinueReadTimeoutTest.java b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/fault/Expect100ContinueReadTimeoutTest.java new file mode 100644 index 000000000000..ef84dda5b247 --- /dev/null +++ b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/fault/Expect100ContinueReadTimeoutTest.java @@ -0,0 +1,220 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.http.nio.netty.fault; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static software.amazon.awssdk.http.SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES; + +import io.netty.bootstrap.ServerBootstrap; +import io.netty.channel.Channel; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelInitializer; +import io.netty.channel.ChannelPipeline; +import io.netty.channel.SimpleChannelInboundHandler; +import io.netty.channel.nio.NioEventLoopGroup; +import io.netty.channel.socket.ServerSocketChannel; +import io.netty.channel.socket.nio.NioServerSocketChannel; +import io.netty.handler.codec.http.HttpRequest; +import io.netty.handler.codec.http.HttpServerCodec; +import io.netty.handler.ssl.SslContext; +import io.netty.handler.ssl.SslContextBuilder; +import io.netty.handler.ssl.SslHandler; +import io.netty.handler.ssl.util.SelfSignedCertificate; +import io.netty.handler.timeout.ReadTimeoutException; +import io.reactivex.Flowable; +import java.nio.ByteBuffer; +import java.time.Duration; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLEngineResult; +import javax.net.ssl.SSLException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.reactivestreams.Publisher; +import software.amazon.awssdk.http.EmptyPublisher; +import software.amazon.awssdk.http.Protocol; +import software.amazon.awssdk.http.SdkHttpFullRequest; +import software.amazon.awssdk.http.SdkHttpMethod; +import software.amazon.awssdk.http.SdkHttpResponse; +import software.amazon.awssdk.http.async.AsyncExecuteRequest; +import software.amazon.awssdk.http.async.SdkAsyncHttpClient; +import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; +import software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient; +import software.amazon.awssdk.http.nio.netty.SdkEventLoopGroup; +import software.amazon.awssdk.utils.AttributeMap; + +/** + * Regression test for the bug where {@code OneTimeReadTimeoutHandler} was added before {@code SslHandler} in the pipeline + * via {@code addFirst}, causing TLS handshake {@code channelRead} events to prematurely remove it. This left no read timeout + * handler active during the wait for a {@code 100 Continue} response, causing the request to hang indefinitely. + * + *

The server deliberately delays the TLS handshake to ensure it is still in progress when the client's + * {@code writeRequest()} adds the timeout handler. Without the fix ({@code addBefore} instead of {@code addFirst}), + * the handler gets removed by handshake data and the test times out. + */ +public class Expect100ContinueReadTimeoutTest { + + private SdkAsyncHttpClient netty; + private Server server; + + @BeforeEach + public void setup() throws Exception { + server = new Server(); + server.init(); + + netty = NettyNioAsyncHttpClient.builder() + .readTimeout(Duration.ofMillis(500)) + .eventLoopGroup(SdkEventLoopGroup.builder().numberOfThreads(2).build()) + .protocol(Protocol.HTTP1_1) + .buildWithDefaults(AttributeMap.builder().put(TRUST_ALL_CERTIFICATES, true).build()); + } + + @AfterEach + public void teardown() throws InterruptedException { + if (server != null) { + server.shutdown(); + } + if (netty != null) { + netty.close(); + } + } + + /** + * Sends a PUT request with {@code Expect: 100-continue} to a TLS server that completes the handshake slowly + * and then never responds. The read timeout must fire. + * + *

Without the fix, the {@code OneTimeReadTimeoutHandler} is removed by TLS handshake data and the request + * hangs until the JUnit {@code @Timeout} kills it. + */ + @Test + public void expect100Continue_slowTlsHandshake_serverNotResponding_shouldTimeout() { + SdkHttpFullRequest request = SdkHttpFullRequest.builder() + .method(SdkHttpMethod.PUT) + .protocol("https") + .host("localhost") + .port(server.port()) + .putHeader("Expect", "100-continue") + .putHeader("Content-Length", "1024") + .build(); + + + CompletableFuture future = sendRequest(request); + assertThatThrownBy(() -> future.get(2, TimeUnit.SECONDS)) + .hasRootCauseInstanceOf(ReadTimeoutException.class); + } + + private CompletableFuture sendRequest(SdkHttpFullRequest request) { + return netty.execute(AsyncExecuteRequest.builder() + .responseHandler(new SdkAsyncHttpResponseHandler() { + @Override + public void onHeaders(SdkHttpResponse headers) { + } + + @Override + public void onStream(Publisher stream) { + Flowable.fromPublisher(stream).forEach(b -> { + }); + } + + @Override + public void onError(Throwable error) { + } + }) + .request(request) + .requestContentPublisher(new EmptyPublisher()) + .build()); + } + + /** + * A TLS server that delays the handshake and never sends an HTTP response. + * The handshake delay ensures the client's {@code writeRequest()} executes while the handshake is still in progress, + * which is the condition required to trigger the bug. + */ + private static class Server extends ChannelInitializer { + private static final long HANDSHAKE_DELAY_MS = 200; + + private ServerBootstrap bootstrap; + private ServerSocketChannel serverSock; + private final NioEventLoopGroup group = new NioEventLoopGroup(); + private SslContext sslCtx; + + void init() throws Exception { + SelfSignedCertificate ssc = new SelfSignedCertificate(); + sslCtx = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()).build(); + + bootstrap = new ServerBootstrap() + .channel(NioServerSocketChannel.class) + .group(group) + .childHandler(this); + + serverSock = (ServerSocketChannel) bootstrap.bind(0).sync().channel(); + } + + @Override + protected void initChannel(Channel ch) { + ChannelPipeline pipeline = ch.pipeline(); + SSLEngine engine = sslCtx.newEngine(ch.alloc()); + pipeline.addLast(new SslHandler(new SlowHandshakeSslEngine(engine))); + pipeline.addLast(new HttpServerCodec()); + pipeline.addLast(new SilentHttpHandler()); + } + + void shutdown() throws InterruptedException { + group.shutdownGracefully().await(); + serverSock.close(); + } + + int port() { + return serverSock.localAddress().getPort(); + } + + /** + * An SSLEngine wrapper that adds a delay to {@code wrap()} during the handshake, + * slowing down the server's TLS responses so the client's handshake stays pending + * while {@code writeRequest()} runs. + */ + private static class SlowHandshakeSslEngine extends DelegatingSslEngine { + SlowHandshakeSslEngine(SSLEngine delegate) { + super(delegate); + } + + @Override + public SSLEngineResult wrap(ByteBuffer[] srcs, int offset, int length, ByteBuffer dst) throws SSLException { + if (getHandshakeStatus() != SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING + && getHandshakeStatus() != SSLEngineResult.HandshakeStatus.FINISHED) { + try { + Thread.sleep(HANDSHAKE_DELAY_MS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + return super.wrap(srcs, offset, length, dst); + } + } + + /** Receives the HTTP request but never responds. */ + private static class SilentHttpHandler extends SimpleChannelInboundHandler { + @Override + protected void channelRead0(ChannelHandlerContext ctx, HttpRequest msg) { + // Intentionally do nothing — simulate server not responding to 100-continue + } + } + } +}