davsclaus commented on code in PR #28362: URL: https://github.com/apache/camel/pull/28362#discussion_r4367827994
########## components/camel-as2/camel-as2-api/src/test/java/org/apache/camel/component/as2/api/AS2ClientConnectionReuseTest.java: ########## @@ -0,0 +1,172 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License 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 org.apache.camel.component.as2.api; + +import java.io.BufferedInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.util.Locale; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.camel.util.IOHelper; +import org.apache.hc.core5.http.ClassicHttpResponse; +import org.apache.hc.core5.http.ContentType; +import org.apache.hc.core5.http.io.entity.EntityUtils; +import org.apache.hc.core5.http.io.entity.StringEntity; +import org.apache.hc.core5.http.message.BasicClassicHttpRequest; +import org.apache.hc.core5.http.protocol.HttpCoreContext; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * The client keeps a connection in its pool only when the partner's response allows it: a connection the partner closes + * must not be leased again, or the next send writes on a closed socket (CAMEL-26110). + */ +class AS2ClientConnectionReuseTest { + + private static final int SENDS = 3; + + private ExecutorService executor; + private ServerSocket serverSocket; + private final AtomicInteger connections = new AtomicInteger(); + + @AfterEach + void stopPartner() { + IOHelper.close(serverSocket); + if (executor != null) { + executor.shutdownNow(); + } + } + + @Test + void connectionClosedByThePartnerIsNotReused() throws Exception { + startPartner("HTTP/1.1 200 OK\r\nConnection: close\r\nContent-Length: 2\r\n\r\nok", true); Review Comment: **Non-blocking: the 2-byte body cannot show a read after close.** `ok` arrives in the same read as the status line, so it is already in the input buffer when the connection is closed. A case with `Connection: close` and a body larger than the 8 KB buffer (for example 64 KB of text) would fail with the current change and pass once the body is read before release. Then `assertEquals(body, EntityUtils.toString(response.getEntity()))` checks it. ########## components/camel-as2/camel-as2-api/src/main/java/org/apache/camel/component/as2/api/AS2ClientConnection.java: ########## @@ -279,16 +275,12 @@ public ClassicHttpResponse execute(ClassicHttpRequest request, HttpClientConnect } } finally { if (endpoint != null) { + // a null keep-alive makes the pool close the connection: a zero or negative one keeps it with no expiry TimeValue keepAlive = null; - if (response != null) { - // Normal happy path: release with computed keep-alive. + if (response != null && DefaultConnectionReuseStrategy.INSTANCE.keepAlive(request, response, httpContext)) { Review Comment: **Blocking: the body is read after the connection is closed.** `send()` releases the endpoint in this `finally` and returns the response. The body is read only afterwards: `AS2ClientManager.send` calls `EntityParser.parseAS2MessageEntity(response)` right after `as2ClientConnection.send(...)`. The client adds no response interceptor that buffers it, so the entity still streams from the connection's socket. With a `null` keep-alive, `PoolingHttpClientConnectionManager.release()` (httpclient5 5.6.5) calls `conn.close(CloseMode.GRACEFUL)`, and `BHttpConnectionBase.close` closes the socket. After that, only the bytes already in the session input buffer can be read. That is at most the 8 KB of `Http1Config.setBufferSize(8 * 1024)`, and only what the first read happened to receive. A signed MDN of a few KB over a real network arrives in several TCP segments. So with a partner that answers `Connection: close`, the MDN of the *current* send can now fail to parse (socket closed or premature end of content), where before only the *next* send could fail. Before this PR the connection stayed open in the pool, so the body was readable. That had its own hazard: a concurrent `send()` could lease the connection while this body was still unread. A fix for both: buffer the entity in the `try`, right after `endpoint.execute(...)` (and after the 407 check), so read errors still surface from `send()` and the connection is released only once the body is read: ```java if (response instanceof ClassicHttpResponse classicResponse && classicResponse.getEntity() != null) { // read the body while the connection is ours: release may close it, or hand it to another send classicResponse.setEntity(new BufferedHttpEntity(classicResponse.getEntity())); } ``` AS2 messages and MDNs are small, so buffering is cheap. `BufferedHttpEntity` keeps the content type and encoding of the entity. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
