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

[miklish]

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).

[stevehu]

@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.