oscerd commented on code in PR #27803:
URL: https://github.com/apache/camel/pull/27803#discussion_r4268832252
##########
components/camel-smpp/src/main/java/org/apache/camel/component/smpp/SmppConnectionFactory.java:
##########
@@ -102,13 +112,14 @@ public Connection createConnection(String host, int port)
throws IOException {
socket.connect(new InetSocketAddress(host, port),
config.getEnquireLinkTimer());
}
- if (config.isUsingSSL() && config.getHttpProxyHost() != null) {
- // Init the SSL socket which is based on the proxy socket
- SSLSocketFactory sslSocketFactory = (SSLSocketFactory)
SSLSocketFactory.getDefault();
- // NOTE: the socket must be closed by the factory method
client.
- SSLSocket sslSocket = (SSLSocket)
sslSocketFactory.createSocket(socket, host, port, true); // NOSONAR
- sslSocket.startHandshake();
- socket = sslSocket;
+ if (config.isUsingSSL()) {
+ if (config.getHttpProxyHost() != null) {
+ // Init the SSL socket which is based on the proxy socket
+ // NOTE: the socket must be closed by the factory method
client.
+ socket = getSslSocketFactory().createSocket(socket, host,
port, true); // NOSONAR
+ configureHostnameVerification(socket);
+ }
+ startHandshake((SSLSocket) socket);
Review Comment:
Good catch, and the timeout is the part I had wrong. Fixed in `dd6a172`
exactly as you wrote it.
I went looking for a test first and the one I wrote initially was worthless:
it asserted the socket was closed after a **certificate mismatch**, and it
passed with `IOHelper.close(socket)` removed. `SSLSocketImpl` closes itself
when it sends a fatal alert, so there was nothing for the fix to do on that
path.
Probing JDK 21.0.10 for which paths actually leak:
| failure | socket closed by the JDK? |
|---|---|
| fatal alert during `startHandshake` (cert mismatch) | yes |
| `SocketTimeoutException` during `startHandshake` | **no** |
| `ConnectException` from `Socket.connect` | **no** |
So the leak is the one you named. One correction to the scope in your
comment: the **CONNECT-request** failures were already covered — `connectProxy`
has its own `catch (RuntimeException)` / `catch (Exception)` that calls
`closeSocket(socket)` before rethrowing, so those never leaked. What the new
catch adds is the timed out connect and the timed out handshake, plus the
window where `getSslSocketFactory().createSocket(socket, host, port, true)`
throws and the plain tunnel socket would otherwise be dropped on the floor. I
narrowed the code comment to match rather than claim all three.
The test is now `closesTheSocketWhenTheHandshakeTimesOut`: a `ServerSocket`
that accepts and parks on a latch without answering the ClientHello,
`enquireLinkTimer` at 1s, and a recording `SSLSocketFactory` (through the
package-private `SmppConnectionFactory(SmppConfiguration, SSLSocketFactory)`
constructor) that keeps the socket it handed out so the test can assert
`isClosed()`. Removing `IOHelper.close(socket)` fails it on all three surefire
runs:
```
[ERROR] Run 1: closesTheSocketWhenTheHandshakeTimesOut:148 the socket must
be closed when createConnection fails ==> expected: <true> but was: <false>
[ERROR] Run 2: ... Run 3: ...
```
154 tests, 4 skipped, green in `components/camel-smpp`.
_Claude Code on behalf of
--
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]
