oscerd commented on code in PR #27803:
URL: https://github.com/apache/camel/pull/27803#discussion_r4269092962
##########
components/camel-smpp/src/main/java/org/apache/camel/component/smpp/SmppConnectionFactory.java:
##########
@@ -102,21 +112,59 @@ 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);
}
return new SocketConnection(socket);
} catch (Exception e) {
+ // no Connection is returned, so jSMPP cannot close the socket
either. The JDK closes it on a fatal
+ // alert but not on a timed out connect or handshake, which leaks
one socket per reconnect attempt
+ // against an SMSC that accepts TCP and then does not answer. On
the proxy path the layered SSLSocket
+ // was created with autoClose=true, so closing it closes the
tunnel underneath as well
+ IOHelper.close(socket);
throw new IOException(e.getMessage(), e);
}
}
+ private SSLSocketFactory getSslSocketFactory() {
+ return sslSocketFactory != null ? sslSocketFactory :
(SSLSocketFactory) SSLSocketFactory.getDefault();
+ }
+
+ /**
+ * Completes the TLS handshake here, on the thread that is creating the
connection, instead of leaving it to the
+ * first read or write.
+ * <p/>
Review Comment:
You are right about the HTML — `<p/>` is a self-closing tag for an element
that cannot be empty, and `<p>` is what Javadoc wants as a paragraph separator.
I am keeping `<p/>` anyway, for three reasons I checked rather than assumed:
**It is the only style this module uses.** In
`components/camel-smpp/src/main/java`:
```
$ grep -rho '\* <p/>' components/camel-smpp/src/main/java/ | wc -l
13
$ grep -rho '\* <p>' components/camel-smpp/src/main/java/ | wc -l
0
```
Applying these three suggestions would make them the only `<p>` in
camel-smpp. `core/camel-api` is 1079 to 42 the same way.
**There is no project-wide convention to appeal to in the other direction.**
Across `core/` and `components/` it is 5432 `<p/>` against 5658 `<p>` — near
enough an even split, so neither form is "the Camel style" repo-wide. That
makes the module-local count the one that decides, the same way CLAUDE.md
resolves the AssertJ-versus-JUnit question by what the surrounding module
already does rather than by which is better in the abstract.
**It cannot break anything.** The root `pom.xml` sets
`<doclint>none</doclint>` (lines 743 and 827), so the check that would flag a
self-closing element is deliberately off. With 5432 of these in the tree it
would have to be.
@davsclaus also said in his review of `dd6a172` that he is fine keeping
`<p/>`.
Worth doing properly as its own change if the project wants one form: a
sweep of all 5432, plus turning the doclint HTML check back on so it stays that
way. Fixing three in a passing PR just moves the inconsistency around.
_Claude Code on behalf of oscerd_
--
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]
