[
https://issues.apache.org/jira/browse/LOG4J2-1731?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15743586#comment-15743586
]
Chris Ribble commented on LOG4J2-1731:
--------------------------------------
I wasn't really sure what order the SocketOptions should be applied in.
Intuitively, it makes sense to apply options before connecting, since some
options might affect socket set up, but previously the code was applying
SocketOptions after connect.
Looking at the JavaDoc for Socket
(https://docs.oracle.com/javase/7/docs/api/java/net/Socket.html) I don't see
anything indicating that you should call the various Socket functions after
connect, but three of them (setTrafficClass, setReuseAddress, and
setPerformancePreferences) indicate that they may have no effect or have
undefined behaviors after binding the socket (depending on the underlying
platform implementation).
I pulled master and ran the tests just now; they pass and so does my use case
(setting timeout still works). However, I'm not specifying any extra
SocketOptions, so I can't really speak to the correctness of the SocketOptions
apply order.
> SslSocketManager should respect connectTimeoutMillis
> ----------------------------------------------------
>
> Key: LOG4J2-1731
> URL: https://issues.apache.org/jira/browse/LOG4J2-1731
> Project: Log4j 2
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.7
> Reporter: Chris Ribble
> Assignee: Remko Popma
> Labels: patch
> Fix For: 2.8
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> Creating a SocketAppender with an SSLConfiguration causes log4j to use
> SslSocketManager to connect to the remote server.
> When SSL is not configured, TcpSocketManager correctly uses
> connectTimeoutMillis both during the initial socket setup and in the
> reconnection manager thread.
> But when SSL is configured SslSocketManager does not use connectTimeoutMillis
> when creating SSL sockets. The affects both the initial connection and the
> reconnection manager thread.
> In my testing, it takes nearly a minute for the connection to time out when
> the other side does not exists (i.e. use invalid host like 10.0.0.0).
> Example code to set up the SocketAppender:
> {code}
> SocketAppender appender = SyslogAppender.newBuilder()
> .withHost("10.0.0.0")
> .withPort(514)
> .withProtocol(Protocol.SSL)
>
> .withSslConfiguration(SslConfiguration.createSSLConfiguration("TLSv1.2",
> null, null))
> .withConnectTimeoutMillis(1000)
> .withReconnectDelayMillis(500)
> .withImmediateFail(true)
> .withImmediateFlush(true)
> .withAdvertise(false)
> .withIgnoreExceptions(true)
> .withLayout(layout)
> .withName("MY_APPENDER")
> .build();
> appender.start();
> {code}
> If you run this, you should see the call to build() block for ~60s until the
> underlying operating system timeout occurs.
> Interestingly, the reconnection code and the initial connection code ignore
> the timeout for different reasons.
> 1. The reconnect code uses a pattern that allows for specification of the
> timeout, but 0 is hard-coded in the constructor call for SslSocketManager.
> 2. The initial connection code doesn't use a pattern that allows for
> specification of the connection time.
> Suggested fixes:
> 1. Use the configuration data's connectTimeoutMillis when instantiating
> SslSocketManager instead of hard-coding timeout to 0
> 2. Use the same pattern that the reconnection code uses for socket connect
> where timeout is specified when connecting the socket.
> Suggested patch:
> {code}
> diff --git
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java
>
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java
> index 57a4b43..8c9eddc 100644
> ---
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java
> +++
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java
> @@ -190,8 +190,8 @@ public class SslSocketManager extends TcpSocketManager {
> LOGGER.catching(Level.DEBUG, e);
> return null;
> }
> - return new SslSocketManager(name, os, socket,
> data.sslConfiguration, inetAddress, data.host, data.port, 0,
> - data.delayMillis, data.immediateFail, data.layout,
> data.bufferSize, data.socketOptions);
> + return new SslSocketManager(name, os, socket,
> data.sslConfiguration, inetAddress, data.host, data.port,
> + data.connectTimeoutMillis, data.delayMillis,
> data.immediateFail, data.layout, data.bufferSize, data.socketOptions);
> }
> private InetAddress resolveAddress(final String hostName) throws
> TlsSocketManagerFactoryException {
> @@ -218,11 +218,12 @@ public class SslSocketManager extends TcpSocketManager {
> SSLSocket socket;
> socketFactory = createSslSocketFactory(data.sslConfiguration);
> - socket = (SSLSocket) socketFactory.createSocket(data.host,
> data.port);
> + socket = (SSLSocket) socketFactory.createSocket();
> final SocketOptions socketOptions = data.socketOptions;
> if (socketOptions != null) {
> socketOptions.apply(socket);
> }
> + socket.connect(new InetSocketAddress(data.host, data.port),
> data.connectTimeoutMillis);
> return socket;
> }
> }
> {code}
> With this patch applied, both the initial connect and reconnect respect
> connectTimeoutMillis and my tests/applications don't hang on startup when the
> server is not available.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org
For additional commands, e-mail: log4j-dev-h...@logging.apache.org
