-
Notifications
You must be signed in to change notification settings - Fork 635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve Consul connection leaks by using SimpleURIConnectionPool #1657
Merged
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
…SimpleConnectionMaker to create connections) to make testing easier
…nnection factory)
…low-level classes to separate package
…es to specify connection timeout
… to SimpleConnection 2. added example mock connection and mock connection maker
…ation exceptions in testing
…created connection
…-synchronized since they are only used in this class and only called directly or transitively by synchronized methods
…y used in this class and only called directly or transitively by synchronized methods
…yword was removed
…timized connection leak detection code
…ing Iterators (to avoid ConcurrentModificationException)
…nnection' in SimpleConnectionHolder. Also, removed redundant log message SimpleConnectionHolder.safeClose()
…tionPool with SimpleURIConnectionPool
miklish
changed the title
Simplepool1645
Resolve Consul connection leaks by using SimpleURIConnectionPool
Mar 16, 2023
miklish
requested review from
stevehu,
AkashWorkGit,
GavinChenYan,
jaydeepparekh1311 and
zabooma
March 16, 2023 16:36
GavinChenYan
approved these changes
Mar 16, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks
AkashWorkGit
approved these changes
Mar 16, 2023
AkashWorkGit
approved these changes
Mar 16, 2023
stevehu
approved these changes
Mar 16, 2023
Thanks, @miklish @GavinChenYan @AkashWorkGit for your help. |
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.
SimpleConnectionPool
SimpleConnectionPool
is a simple, natively mockable, threadsafe, resilient, high-performance connection pool intended to replacecom.networknt.client.http.Http2ClientConnectionPool
.Use of this connection pool fully resolves issue 1656 (#1656) by replacing the use of
Http2ClientConnection
incom.networknt.consul.client.ConsulClientImpl.lookupHealthService()
withSimpleURIConnectionPool
.Focus on Stability and Testability
The primary goal of SimpleConnectionPool is operational stability and reliability. Optimizations were considered only after stability and resilience were confirmed.
Easy testability was vital to confirm the correct behaviour of the connection pool under a wide array of common and corner-case conditions. To this end, the connection pool was developed with mockability built-in.
Avoidance of Consul Connection Bug
The following Consul connection-leak bug was reported in August 2020, but has yet to be resolved.
SimpleConnectionPool avoids this bug entirely by never closing connections that are in use. While this can cause connection expiry times to not be precisely honoured, it also ensures that Consul blocking queries are never canceled before they complete, thereby avoiding the conditions that manifest this bug.
Multi-Layer Architecture
The connection pool was developed using a multilayer architecure.
Level 1: Routing to URI connection pools
Threads that need a connection to a URI must 'borrow' a connection from the pool and then
restore
that connection to the pool when they are done with it.A
SimpleConnectionPool
routes these URI-connection requests to aSimpleURIConnectionPool
for that URI. If aSimpleURIConnectionPool
does not exist for that URI, theSimpleConnectionPool
will create one.The
SimpleConnectionPool
needs only minimal thread synchronization since theSimpleURIConnectionPool
s are already fully thread safe. A benefit of this, is increased opportunity for concurrency. For example: N threads can request connections to N distinct URIs concurrently.Level 2: URI connection pools
A
SimpleURIConnectionPool
manages connections to a single URI where the connections it manages have the ability to be used by 1 or more threads concurrently. ASimpleURIConnectionPool
:Also, note that
SimpleURIConnectionPool
s can be used used independently ofSimpleConnectionPool
. This means that, any code that only needs to connect to a single URI can use aSimpleURIConnectionPool
directly--it does not need to make requests to borrow connections from aSimpleConnectionPool
(which is only needed for code that needs to connect to multiple distinct URIs).The
SimpleURIConnectionPool
needs very little information about connections to manage the pool. It only needs to know all of the following boolean properties of the connection:Level 3: Connection Holders
SimpleConnectionHolder
objects translate the state of connections into the smaller set of states relevant to theSimpleURIConnectionPool
(namely borrowed, borrowable, expired, closed).SimpleConnectionHolder
s have a 1-1 relationship with connections, meaning that every connection in the pool is contained by a singleSimpleConnectionHolder
, and everySimpleConnectionHolder
contains exactly one connection. In the docs, we therefore often refer to connections and their connection holders interchangeably.SimpleConnectionHolder
s also keep track of whether a connection is currently in use or not through the use ofConnectionToken
objects that client threads must 'borrow' in order to get a connection, and must 'restore' when they are done with the connection.When a
SimpleConnectionHolder
is created, it also creates the connection it holds. It does this by using aSimpleConnectionMaker
factory object that is passed to it in its constructor.The connection state exposed by the
SimpleConnectionHolder
adheres to the following state diagram.State diagram for a connection
Level 4: SimpleConnections
Objects that implement
SimpleConnection
interfaces must wrap a physical 'raw' connection. For example, theSimpleClientConnection
class implementsSimpleConnection
and wraps Undertow'sClientConnection
.SimpleConnectionHolder
s only deal withSimpleConnection
objects--they never deal directly with 'raw' connections (such as Undertow'sClientConnection
).Mockability
Since
SimpleConnectHolder
objects use aSimpleConnectionMaker
to create their connections, and since the connections thatSimpleConnectionMakers
create areSimpleConnections
, it means that theSimpleConnectionPool
,SimpleURIConnectionPool
, andSimpleConnectionHolder
are able to manage the pool without having any knowledge of the raw connections they are managing.This means connections can be fully mocked by simply implementing
SimpleConnection
and creating aSimpleConnectionMaker
that instantiates these mock connections.Mock Test Harness
SimpleConnectionPool comes with a test harness that can be used to easily test mock connections. For example, to test how the connection pool will handle a connection that randomly closes can be built as follows:
1. Develop the mock connection
2. Test how the connection pool handles the connection
Plugability
SimpleConnectionPool
requires very litte information about a connection in order to manage that connection. This lends itself to supporting many different networking APIs. If one can implement aSimpleConnection
andSimpleConnectionMaker
using a networking API, then connections created using that API can be managed bySimpleConnectionPool
.ClientConnections
created using undertow's libraries can be wrapped this way. See thesimplepool.undertow
package to see how support for undertow was implemented.How to safely use the SimpleURIConnectionPool
The following code snippet demonstrates the recommended way to structure code that borrows a connection...
...and this demonstrates the recommended way to borrow connections in a loop...