Add connection timeout to TLS Channel #1686
Conversation
JAVA-5856
|
|
||
| SSLParameters sslParameters = sslEngine.getSSLParameters(); | ||
| enableSni(getServerAddress().getHost(), sslParameters); | ||
| group.getTimeoutExecutor().schedule(() -> { |
There was a problem hiding this comment.
For better scalability, I believe we should use a dedicated timeout executor for connection timeouts.
There was a problem hiding this comment.
Isn't group.getTimeoutExecutor() dedicated to timeouts?
|
|
||
| SSLParameters sslParameters = sslEngine.getSSLParameters(); | ||
| enableSni(getServerAddress().getHost(), sslParameters); | ||
| group.getTimeoutExecutor().schedule(() -> { |
There was a problem hiding this comment.
//TODO connectTimeoutMs being 0 - should not schedule timeout interruption.
JAVA-5856
| @@ -121,23 +144,29 @@ private Pair(final SocketChannel socketChannel, final Runnable attachment) { | |||
| } | |||
| } | |||
|
|
|||
| // Monitors OP_CONNECT events. | |||
There was a problem hiding this comment.
Should this rather be documented on SelectorMonitor via a documentation comment, as opposed to being a free-floating code comment?
| enum ConnectionRegistrationState { | ||
| CONNECTING, | ||
| CONNECTED, | ||
| TIMEOUT_OUT |
|
|
||
| private Pair(final SocketChannel socketChannel, final Runnable attachment) { | ||
| enum ConnectionRegistrationState { |
There was a problem hiding this comment.
ConnectionRegistrationState is not used outside of SocketRegistration.
| enum ConnectionRegistrationState { | |
| private enum ConnectionRegistrationState { |
| SSLEngine sslEngine = getSslContext().createSSLEngine(getServerAddress().getHost(), | ||
| getServerAddress().getPort()); | ||
| sslEngine.setUseClientMode(true); | ||
| int connectTimeoutMs = getSettings().getConnectTimeout(TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Both SocketStream and NettyStream use TimeoutContext.getConnectTimeoutMs. It seems we should use the same in TlsChannelStream.
int connectTimeoutMs = operationContext.getTimeoutContext().getConnectTimeoutMs();Furthermore, TimeoutContext.getConnectTimeoutMs requires us to handle MongoOperationTimeoutException, provided that we plan to throw InterruptedByTimeoutException instead of it, as the closeAndTimeout method does.
There was a problem hiding this comment.
Good catch! I’ve updated it to use TimeoutContext.getConnectTimeoutMs.
Regarding the handling - both SocketStream and NettyStream propagate the exception down the stack without catching it in stream.openAsync. InternalStreamConnection.openAsync wraps stream.openAsync in a try/catch and handles MongoOperationTimeoutException there:
Generally, I think that this exception should be handled in AsyncCompletitionHandler rather then synchronous try/catch around stream.openAsync (currently some are thrown from stream.openAsync and some are propagated to a handler) - we could reconsider this behavior as part of the async refactoring epic.
NettyStream and SocketStream throw MongoOperationTimeoutException as is, because "connection process" has not started yet. I moved operationContext.getTimeoutContext().getConnectTimeoutMs(); before socketChannel.connect(getSocketAddresses(getServerAddress(), inetAddressResolver).get(0)); to align with the same behaviour. In TlsChannelStream case, MongoOperationTimeoutException is propagated to the handler. Let me know what you think.
driver-core/src/main/com/mongodb/internal/connection/TlsChannelStreamFactoryFactory.java
Outdated
Show resolved
Hide resolved
| this.connectionRegistrationState = new AtomicReference<>(ConnectionRegistrationState.CONNECTING); | ||
| } | ||
|
|
||
| public boolean markConnectionEstablishmentTimedOut() { |
There was a problem hiding this comment.
| public boolean markConnectionEstablishmentTimedOut() { | |
| boolean markConnectionEstablishmentTimedOut() { |
| ConnectionRegistrationState.TIMEOUT_OUT); | ||
| } | ||
|
|
||
| public boolean markConnectionEstablishmentCompleted() { |
There was a problem hiding this comment.
| public boolean markConnectionEstablishmentCompleted() { | |
| boolean markConnectionEstablishmentCompleted() { |
| enableSni(getServerAddress().getHost(), sslParameters); | ||
| private void scheduleTimeoutInterruption(final AsyncCompletionHandler<Void> handler, | ||
| final SelectorMonitor.SocketRegistration socketRegistration, | ||
| final SocketChannel socketChannel, |
There was a problem hiding this comment.
This method should not accept socketChannel because socketRegistration already contains it.
| //TODO refactor ths draft | ||
| InterruptedByTimeoutException timeoutException = new InterruptedByTimeoutException(); | ||
| try { | ||
| socketChannel.close(); |
There was a problem hiding this comment.
This is the only place where we close the SocketChannel opened in TlsChannelStream.openAsync. Shouldn't we close all the SocketChannels registered with SelectorMonitor, and do it in a way that makes sure than none of them stays open due to a race condition?
| enum ConnectionRegistrationState { | ||
| CONNECTING, | ||
| CONNECTED, | ||
| TIMEOUT_OUT | ||
| } |
There was a problem hiding this comment.
I propose a change that does not involve introducing ConnectionRegistrationState, and instead of boolean markConnectionEstablishmentTimedOut/boolean markConnectionEstablishmentCompleted methods exposes boolean tryCancel/void runIfNotCancelled on SocketRegistration: stIncMale@f4e9f37.
What do you think?
P.S. I made the change blindfolded, without running any tests.
|
The last reviewed commit is 238268c. |
…lStreamFactoryFactory.java Co-authored-by: Valentin Kovalenko <[email protected]>
JAVA-5856
JAVA-5856