DefaultManagedHttpClientConnection: Restore original socket timeout

[rschmitt]

This change fixes a bug in the synchronous client where socket timeouts were not being set correctly on reused connections, and the client was instead setting it to either the TLS handshake timeout or the responseTimeout from the RequestConfig of the previous request.

The fix works by changing DefaultManagedHttpClientConnection to store the socketTimeout set on the socket at the time bind is called, and always restore that value when the activate method is called. Note that this requires the caller to call setSoTimeout on the socket before binding the connection to it, hence the adjustments to some of the code in DefaultHttpClientConnectionOperator.

[ok2c]

@rschmitt Why is this necessary? SocketConfig defines the initial socket parameters only. It should have no bearing on the persistent connections once they have been initialized. This change will also likely make behavior of the classic connection manager inconsistent with that of the async one.

If users want specific socket timeout for their persistent connections they should be using ConnectionConfig

[rschmitt]

SocketConfig defines the initial socket parameters only.

Strictly speaking, this is part of the stated contract for many of the methods in SocketConfig, but not setSoTimeout:

• setSoLinger: "Determines the default value of the {@link SocketOptions#SO_LINGER} parameter for newly created sockets."
• setSoReuseAddress: "Determines the default value of the {@link SocketOptions#SO_REUSEADDR} parameter for newly created sockets."
• setSoTimeout: "Determines the default socket timeout value for blocking I/O operations."

The failure to re-set SocketConfig.soTimeout on reused connections is a behavior change in 5.6 that I learned about from a user whose integration tests started failing. I interpret it as a regression.

It should have no bearing on the persistent connections once they have been initialized.

Why is the socket timeout getting changed in the first place? I don't actually know what's changing it or why.

This change will also likely make behavior of the classic connection manager inconsistent with that of the async one.

There was no such regression in the async manager, so there's already an inconsistency somewhere; the sync manager is screwing with the socket timeout when the async manager isn't.

[ok2c]

@rschmitt I cannot agree with that. I see no good reason to make behavior of this class illogical and inconsistent because there is (or was) supposedly another inconsistency somewhere else.

Again, if people want specific socket timeout they should explicitly set it with ConnectionConfig. If they leave it null they are fine with it being undefined.

[rschmitt]

We have to do something, we can't just set users up to fail. Can we deprecate the setSoTimeout methods in IOReactorConfig and SocketConfig?

[ok2c]

@rschmitt Why should we deprecate #setSoTimeout methods in IOReactorConfig and SocketConfig? What is wrong with them? Those are core level configuration that ensure connect initialization of newly established connections.

[rschmitt]

Let me look into this some more. For one thing, it appears that the responseTimeout on RequestConfig is leaking across requests.

[ok2c]

@rschmitt The only way to resolve this problem in a reasonable manner if you do not want the users to make the decision is to make than decision for them by using non-null socket timeout in ConnectionConfig.

[rschmitt]

I think the issue is in DefaultHttpClientConnectionOperator:

final int soTimeout = socket.getSoTimeout();
final Timeout handshakeTimeout = tlsConfig.getHandshakeTimeout() != null ? tlsConfig.getHandshakeTimeout() : connectTimeout;
if (handshakeTimeout != null) {
    socket.setSoTimeout(handshakeTimeout.toMillisecondsIntBound());
}
final SSLSocket sslSocket = tlsSocketStrategy.upgrade(socket, tlsName.getHostName(), tlsName.getPort(), attachment, context);
conn.bind(sslSocket, socket);
socket.setSoTimeout(soTimeout);

These last two lines are in the wrong order. DefaultManagedHttpClientConnection#bind stores sslSocket.getSoTimeout(), which is still the handshake timeout:

socketTimeout = Timeout.ofMilliseconds(sslSocket.getSoTimeout());

Then, we go behind the conn's back and set the timeout directly on the socket:

socket.setSoTimeout(soTimeout);

After the connection is bound, the correct way to do this would be to set the socket timeout through the connection, which updates the conn.socketTimeout field:

conn.setSocketTimeout(Timeout.ofMilliseconds(soTimeout));

So anyway, when the connection gets leased from the pool, the stale socketTimeout (which is actually the TLS handshake timeout from the last request) is reapplied by activate():

@Override
public void activate() {
    super.setSocketTimeout(socketTimeout);
}

There's a similar bug where the RequestConfig.responseTimeout, if set, gets captured by conn and reapplied by activate(). I'm not immediately sure how to fix that, but it's also a less impactful bug: you'd have to send one request with a responseTimeout set, and then a subsequent request without one. (My test does exactly this, but that's pretty artificial.)

[rschmitt]

OK, a minimal change to fix responseTimeout leaking across requests is to change DefaultManagedHttpClientConnection#setSocketTimeout to simply not update this.socketTimeout. In other words, the socketTimeout would be captured at bind time and not updated after that, thus ensuring that activate() always restores the connection default socket timeout and not a request-scoped override value from RequestConfig. I have to slightly rework UDS connection establishment, but it seems to work.