Skip to content
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

Unsafe threading and failure-causing states caused by making SimpleConnectionHolders accessible to users #2101

Open
miklish opened this issue Feb 2, 2024 · 1 comment

Comments

@miklish
Copy link
Collaborator

miklish commented Feb 2, 2024

PR: #2258

By breaking encapsulation and allowing users direct access to the internal state of SimpleURIConnectionPool, thread safety is broken.

The original (pre-port) design of SimpleURIConnectionPool purposely prevented user access to its SimpleConnectionHolders to prevent their modification outside the synchronization of SimpleURIConnectionPool.borrow() and .restore() methods.

This will also break the SimpleURIConnectionPool's internal connection management since users can borrow directly from SimpleConnectionHolders without the connection pool knowing: if the pool does not know the connection holder has been borrowed, the pool will not move it into the correct state-set inside the SimpleURIConnectionPool which will put the pool into an inconsistent state leading to errors (such as allowing the same HTTP/1.1 connection to be borrowed by multiple threads at the same time).

Another way the internal consistency of the pool can be broken is via the use of SimpleConnectionHolder.safeClose(long now). The now time value is intended to ensure the connection expiry boolean remains constant throughout the entire length of a SimpleURIConnectionPool.borrow() or a SimpleURIConnectionPool.restore(). By allowing calls to SimpleConnectionHolder.safeClose(long now) outside of the synchronization of SimpleURIConnectionPool.borrow() and .restore() -- and using a now value arbitrarily set by the user -- this internal assumption of the pool is broken. The now time value should never be made accessible to users of the pool.

Issue #1788 should be reverted to restore the thread safety and correctness of the connection pool algorithm.

If the purpose of this change was to allow users to safely close connections then a new method outside the simplepool package can be created to do so in a way that does not break the design of the pool.

SimplePool was designed to gracefully handle unexpected connection closures... there is no need to expose its internal state to safely close connections.

forceExpire()

If the goal of the change was to prevent shared connections from being closed while they are still in use by other threads, then a new option under development can resolve that issue. A new forceExpire() method on the ConnectionToken class will indicate to the pool to immediately (in a state-safe and thread-safe manner) close a connection as soon as it reaches 0 borrows (even if it would otherwise still have time before naturally expiring).

@miklish miklish changed the title Unsafe threading and failure-causing states caused by making SimpleConnectionHolder accessible to users Unsafe threading and failure-causing states caused by making SimpleConnectionHolders accessible to users Feb 2, 2024
@stevehu
Copy link
Contributor

stevehu commented Feb 2, 2024

@miklish Thanks a lot for pointing out the potential issues for the PR. As you have guessed, I have exposed those two methods to restore the connection after it is used. I will look at the code again to see if I can add a new method outside the simplepool package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants