Add a dedicated method for disconnecting TLS connections #10005
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
All usages of `AsioTlsStream` were already using `Shared<AsioTlsStream>` to keep a reference-counted instance. This commit moves the reference counting to `AsioTlsStream` itself by inheriting from `SharedObject`. This will allow to implement methods making use of the fact that these objects are reference-counted. The changes outside of `lib/base/tlsstream.hpp` are merely replacing `Shared<AsioTlsStream>::Ptr` with `AsioTlsStream::Ptr` everywhere.
Calling `AsioTlsStream::async_shutdown()` performs a TLS shutdown which exchanges messages (that's why it takes a `yield_context`) and thus has the potential to block the coroutine. Therefore, it should be protected with a timeout. As `async_shutdown()` doesn't simply take a timeout, this has to be implemented using a timer. So far, these timers are scattered throughout the codebase with some places missing them entirely. This commit adds helper functions to properly shutdown a TLS connection with a single function call.
This new helper functions allows deduplicating the timeout handling for `async_shutdown()`.
This new helper function has proper timeout handling which was missing here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Properly closing a TLS connection involves sending some shutdown messages so that both ends know that the connection wasn't truncated maliciously. Exchanging those messages can stall for a long time if the underlying TCP connection is broken. The HTTP connection handling was missing any kind of timeout for the TLS shutdown so that dead connections could hang around for a long time.
This PR introduces two new methods on
AsioTlsStream, namelyForceDisconnect()which just wraps the call for shutting down the TCP connection andGracefulShutdown()which performs the TLS shutdown with a timeout just like it was done inJsonRpcConnection::Disconnect()before.As the lambda passed to
Timeouthas to keep the connection object alive,AsioTlsStreamis changed to inherit fromSharedObject, adding reference counting to it directly. Previously, it was already created asShared<AsioTlsStream>everywhere. Thus, a good part of the first commit is changing that type across multiple files.Open questions:
ForceDisconnect()directly in case a connection is shut down due to a timeout like "no messages received" on JSON-RPC connections?async_shutdown()with calls to the newGracefulShutdown(). In both instances, those calls are already guarded by higher-level timeouts (apilistener.cpp, apilistener.cpp, ifwapichecktask.cpp), the idea for replacing them would be to have just a single call toasync_shutdown()in our code base that's properly guarded.fixes #9986