-
Notifications
You must be signed in to change notification settings - Fork 633
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
Convenience methods to close SimplePool connections #2258
Merged
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
…iately, or as soon as the connection is no longer borrowed
…borrowed after a call to restoreAndScheduleClose
… The generic MockKeepAliveConnection class is adequate for this test
…g or scheduling a safeClose
…must still be restored to the ppol
… connection source-port
…ER it had already happened
…or a connection that was already-removed by another thread
… the connection token: 'It is NOT necessary to use this method to close connections. SimpleConnectionPool and * SimpleURIConnectionPool are specifically designed to handle unexpected connections closures'
…not SimpleUndertowConnectionMaker.getInstance()
…lso, clarified / cleaned code for safeClose()
…heduleSafeClose()
jaydeepparekh1311
approved these changes
Jun 11, 2024
… is null, and (2) rename randomReconnectJitterMs -> randomWaitTimeJitterMs for consistency in the wait() method
AkashWorkGit
approved these changes
Jun 12, 2024
mihai-vladuc
approved these changes
Jun 12, 2024
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.
Issue: #2101
This PR adds two convenience methods to allow closing connections immediately (
safeClose()
), or expiring connections immediately (scheduleSafeClose()
).safeClose()
This is a convenience method that immediately closes a connection even if there are still threads actively using it (i.e: it will be closed even if it is currently borrowed by multiple threads).
Note
Again, we stress that this method is purely for convenience. Users can safely close raw connections directly at any time without calling this method since SimpleConnectionPool was designed to safely handle unexpected closures of raw connections. There is no "safety" benefit to using SimplePool to close connections versus closing them directly, and there never has been. For example:
This means that users can leverage
safeClose()
to close connections if it is convenient, but can also close the raw connection directly at any time if that is more convenient.scheduleSafeClose
This convenience method causes the connection to be changed to an
expired
state which (1) immediately makes the connection unborrowable, and (2) ensures that the connection is immediately closed after all threads currently using it have restored it to the pool. This prevents threads that are currently using it from experiencing unexpected connection closures, while also ensuring that the connection is never reused.Warning
Overuse of either of these methods will negate all benefits of the connection pool. Under normal circumstances, users should never close connections themselves (either via these methods or directly via the raw connection) but instead, always let the connection pool handle all connection closures.
Manually closing connections negates all benefits of using a connection pool. Users should be certain that these method are only used in cases where there is a need to ensure a connection is not reused.
Revert "Fix" for Issue 1788
The code changes made to address Issue 1788 (light-4j 2.x) had the reverse of its intended effect by actually creating an unsafe situation where none had existed before, by breaking the pool's thread safety and breaking the encapsulation of the pool's internal state.
SimplePool was already designed to safely handle the case where its connections were closed directly -- there was absolutely no benefit to "safety" in using SimplePool to close connections, since closing connections directly was already completely safe. This nullifies the very purpose of Issue 1788 (light-4j 2.x) entirely.
The changes in this PR (once ported to light-4j 2.x) will hopefully allow Issue 1788 (light-4j 2.x) to be promptly reverted by providing methods in SImplePool to close connections, since (as mentioned) the "fix" for Issue 1788 dangerously breaks the encapsulation and thread safety of the pool and can (among other issues) cause the pool to allow multiple threads to simultaneously write to the same (non-multiplexed) HTTP/1.1 connection (which would result in intricate, intermittent, and hard to diagnose connection failures).
Again, it is stressed that the methods added in this PR are added purely for convenience, since they are *not needed for any reasons related to safety.