Fix #6125: use the Expect: 100-continue handler of the pipeline on po…

[abelet]

1 Summary

When the Expect: 100-continue mechanism is used with chunked transfer encoding, the server receives two requests for one JAX-RS call: an empty one and the complete one.

This happens for the following reason: NettyConnector creates a new JerseyExpectContinueHandler instance for every request. If the request carries an Expect: 100-continue header, the connector gives a latch to this instance and waits on that latch. The latch can be released by the handler that receives the 100 Continue response. To receive the 100 Continue response, the handler must be in the pipeline. But the handler is only added to the pipeline when a new channel is created. On a channel taken from the pool, the pipeline holds a different handler: the one that was created for the first request on that channel. That handler has no latch, so it releases nothing.

As a result, every Expect: 100-continue request sent on a reused channel waits until the timeout expires, even if the 100 Continue response arrives immediately. The connector handles the timeout by writing an empty last chunk, so the server sees an empty finished request. After writing the last chunk, the connector sends the request again without the Expect: 100-continue header, as a fallback. This is how one JAX-RS call becomes two requests.

2 Environment

• Jersey 3.1.11 (jersey-netty-connector), Netty 4.1.122.Final, JDK 21
• Affected versions: 2.47 and later, 3.0.18 and later, 3.1.11 and later, and the 4.0.x releases. The last releases without this behaviour: 2.46, 3.0.17 and 3.1.10.

3 Steps to reproduce

Conditions for reproducing the problem:

• In BUFFERED mode the Expect: 100-continue header is not put on the request, so the problem cannot be reproduced.
• The server has to keep the connection alive.

The program below sends four PUT requests in a row to the same dedicated server. A parameter is used to control how the body is sent: chunked transfer encoding or a Content-Length header that is set by the caller. The first request creates a new channel; the others take a channel from the pool.

// run: java Repro chunked        -> Transfer-Encoding: chunked
//      java Repro content-length -> Content-Length header
boolean chunked = args.length > 0 && "chunked".equals(args[0]);

ClientConfig config = new ClientConfig()
        .connectorProvider(new NettyConnectorProvider())
        .property(ClientProperties.EXPECT_100_CONTINUE, true);
if (chunked) {
    config.property(ClientProperties.REQUEST_ENTITY_PROCESSING, RequestEntityProcessing.CHUNKED);
}

Client client = ClientBuilder.newClient(config);
String base = "http://localhost:8080";       // the server with the endpoint below
byte[] body = new byte[256 * 1024];          // above EXPECT_100_CONTINUE_THRESHOLD_SIZE

for (int i = 1; i <= 4; i++) {
    Invocation.Builder request = client.target(base + "/upload-" + i).request();
    if (!chunked) {
        request.header(HttpHeaders.CONTENT_LENGTH, body.length);
    }
    try (Response r = request.put(Entity.entity(body, MediaType.APPLICATION_OCTET_STREAM_TYPE))) {
        System.out.println(i + " -> " + r.getStatus());
    }
}

The requests go to this JAX-RS endpoint:

@Path("/")
public class Upload {
    @PUT
    @Path("{name}")
    public Response put(@PathParam("name") String name, byte[] body) {
        System.out.println("server received: PUT /" + name + ", " + body.length + " bytes");
        return Response.ok().build();
    }
}

The endpoint was deployed on Tomcat 11 and on Jetty 12, with the same result on both:

	The body of the request	Result
1	Content-Length (as in the code above)	All four requests get a 200 OK response, but requests 2–4 need about 500 ms more to be sent.
2	chunked (REQUEST_ENTITY_PROCESSING=CHUNKED, without a Content-Length header)	The server receives seven requests for the four calls: the first one with the complete body (new channel used), then pairs of an empty and a complete request.

With #6040 patched locally, setting 1 also causes data corruption, as the "Actual behaviour" section describes.

4 Expected behaviour

When the server sends a 100 Continue response, the connector should act on it and send the body of the request. This should happen for every request, not only for the first request on the channel.

5 Actual behaviour

When the behaviour is examined in a dedicated environment, also on one thread and with one channel, the problem looks like a slowdown only: the first request is fast, but all the other requests take about 500 ms longer. (The 500 ms is not a delay in sending: it is the EXPECT_100_CONTINUE_TIMEOUT expiring.)

What the server receives depends on the way the body is sent:

• With chunked transfer encoding: The server receives the request twice (the first is empty).

• For a request that is sent with a Content-Length header: The server receives the request data correctly due to another bug, described in #6040.

• With a Content-Length header, after #6040 is fixed: The server receives the repeated request line and headers as the first bytes of the body of the request. The end of the request body remains in the open socket and ends up at the beginning of the next request.

6 Details of the problem

NettyConnector.execute creates a JerseyExpectContinueHandler handler for every request, but puts this handler into the pipeline with the p.addLast(EXPECT_100_CONTINUE_HANDLER, expect100ContinueHandler) call only when a new channel is created.

For every request that carries an Expect: 100-continue header, the connector creates a CountDownLatch and attaches it (with attachCountDownLatch) to the JerseyExpectContinueHandler handler instance created for the request, and then it waits on that latch for the response. The connector reads the response (with processExpectationStatus) from the handler created for the request after the supposed release of the latch.

When the channel is from the pool, the release of the latch never happens, because the response is processed by the handler in the pipeline, which did not get the latch. There are two handler instances: one is actually processing the data, and the other is used by the connector. Because the latch is never released, this leads to a timeout and a fallback behaviour in the connector: it resends the request without the Expect: 100-continue header. The exact behaviour depends on the way the body is sent:

• With chunked transfer encoding: When the timeout happens, the connector writes an EMPTY_LAST_CONTENT element to the channel. In the ST_CONTENT_CHUNK state it is encoded as a terminating chunk (0\r\n\r\n), so the server sees a finished request with an empty body: it processes the request and sends a response. The fallback request then arrives with the complete body, and the server processes it as a separate request. One JAX-RS call becomes two requests, and the server executes both of them.
• For a request that is sent with a Content-Length header: Due to #6040 the EMPTY_LAST_CONTENT was not writen to the channel at the end of the previous request, so the encoder stayed in the ST_CONTENT_NON_CHUNK state. Because of this, the request line and the headers of the Expect: 100-continue request are not sent. When the timeout happens, the connector writes an EMPTY_LAST_CONTENT element to the channel (fixing what #6040 left out), and it resets the state of HttpClientCodec.Encoder to ST_INIT. The fallback request is then sent correctly, only 500 ms later. The server sees one request, without the Expect: 100-continue header.
• With a Content-Length header, after #6040 is fixed: The request that carries the Expect: 100-continue header is sent; the server answers with 100 Continue, and then it waits for the body only. So it reads the request line and the headers from the fallback request as the first bytes of the original request's body. When the server has read the body up to the length in Content-Length, it leaves the remaining data in the socket. This data then ends up at the beginning of the next request, which the server either rejects or processes with an invalid method name.

The incorrect behaviour was caused by the changes in PR #5847. Before that, the handler was also added to channels that come from the pool, with the addLast(EXPECT_100_CONTINUE_HANDLER, expect100ContinueHandler) call.

As a result of PR #5847, one handler is used during the whole lifetime of the channel: when it has nothing to do, it passes the messages on, but in the case of a 417 or 405 response it has to follow the traffic until the end of the response. In the modified handler, the resetHandler() method makes it possible to use the same instance for the next request. However, for the channels taken from the pool, the handler in the channel pipeline is not used, and resetHandler() is not called. Instead, a new handler is created, and this is where the problem comes from.

7 Possible fix

The problem can be fixed with a tiny change in three places. The fix is demonstrated on the 3.1.11 tag. The necessary changes are the following:

1. Creating the handler instance — NettyConnector.java line 258
For channels from the pool, the handler in the channel pipeline must be used. A handler should be created only if there is no channel (chan == null):

// instead of line 258
final JerseyExpectContinueHandler pooledHandler =
        chan == null ? null : chan.pipeline().get(JerseyExpectContinueHandler.class);
final JerseyExpectContinueHandler expect100ContinueHandler =
        pooledHandler == null ? new JerseyExpectContinueHandler() : pooledHandler;

2. Resetting the handler — NettyConnector.java lines 502–503
The reused handler instance holds a state that belongs to the processing of the previous request, so it has to be reset with the resetHandler() call before the latch is attached:

final CountDownLatch expect100ContinueLatch = new CountDownLatch(1);
expect100ContinueHandler.resetHandler();   // new line
expect100ContinueHandler.attachCountDownLatch(expect100ContinueLatch);

3. Making the reset complete — JerseyExpectContinueHandler.java lines 129–131
The resetHandler() method should also clear the status, the currentState and the propagateLastMessage flag:

void resetHandler() {
    latch = null;
    status = null;                           // new line
    currentState = ExpectationState.IDLE;    // new line
    propagateLastMessage = false;            // new line
}

Fixes #6125