[
https://issues.apache.org/jira/browse/HTTPCLIENT-948?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12874586#action_12874586
]
Sam Berlin commented on HTTPCLIENT-948:
---------------------------------------
Excellent find, Oleg! I just tried navigating through the classes to get a
feel for what's involved, and man... is that code super complex!
Can the HttpConnection escape into user-code anywhere? If so, I think the
problem may extend into the fact that the "AbstractClientConnAdapter"
subclasses do not call super.abortConnection in their close/shutdown methods.
Otherwise, I think it is possible that an HttpConnection that user has captured
(that may be lingering as 'free') can have its state & connection shutdown (by
the user calling 'close' or 'shutdown' on the connection), but the
BasicPoolEntry will be left up-for-grabs by ConnPoolByRoute.getFreeEntry.
Normally this would resolve itself the next time closeExpiredConnections or
closeIdleConnections was called, because it would iterate through entries and
delete them... but there would be a period of time where a shutdown entry is
available. This is a very similar problem to what you describe above.
I think there a few things here that can be fixed, in a few different ways:
1) If it is not safe to link HttpConnection.abort or HttpConnection.shutdown to
HttpConnection.abortConnection (as described above), then we can make
ConnPoolByRoute.getFreeEntry (or more specifically,
RouteSpecificPool.allocEntry) more robust: It can inspect the entries it's
about to return and, if they're closed or otherwise unusable, it deletes that
entry and continues iterating. (This is safe, because in ConnPoolByRoute, if
no entries are returned, the getEntryBlocking method will just create a new one
if there's enough capacity.) The tricky thing here will be keeping the state
in sync between CPBR & RSP, so maybe it is better to leave RSP alone and make
CPBR.getFreeEntry inspect the result RSP.allocEntry, and if it is
closed/unusable, then it just calls deleteEntry on itself (which will clean it
all up properly), and then continue iterating looking for a good connection.
2) Regardless of point 1), I think CPBR.getFreeEntry needs to add a 'wrapped'
HttpConnection to IdleConnectionHandler. That is, a connection that when it is
closed or aborted, it will also shutdown the BasicPoolEntry it is attached to.
Normally this would be handled by the AbstractClientConnAdapter subclasses, but
the HttpConnection that is given to the IdleConnectionHandler is not the
'attached' form of the connection, it is the plain old
DefaultHttpClientConnection. I do not think we can get the full attached form
of the http connection here, so our next best option is to create a fake
HttpConnection (or, better yet, a 'Closeable' and change IdleConnectionHandler
to work on 'Closeable's). The fake connection would override shutdown & close
in order to also shutdown the BasicPoolEntry.
3) As an alternative to 2), ConnPoolByRoute can maintain an additional Map from
Connection -> BasicPoolEntry. The 'closeIdleConnections' &
'closeExpiredConnections' methods in IdleConnectionHandler can take an
additional Map<HttpConnection, BasicPoolEntry> and upon closing a connection
they would do a lookup in the map & shutdown the entry. In additional to that,
to be extremely safe, the IdleConnectionHandler could also take a an
AbstractConnPool parameter and call freeEntry on the entry it expired, so that
it's immediately removed. (This last part wouldn't be necessary if 1) were
implemented. It would also significantly increase the amount of locks/unlocks
performed.)
I think the best option is to do 1) & 2). 1) makes getFreeEntry a lot safer,
and 2) is a very straightforward approach. 3) seems like a lot of overhead.
> IdleConnectionHandler can leave closed connections in a inconsistent state
> --------------------------------------------------------------------------
>
> Key: HTTPCLIENT-948
> URL: https://issues.apache.org/jira/browse/HTTPCLIENT-948
> Project: HttpComponents HttpClient
> Issue Type: Bug
> Components: HttpConn
> Affects Versions: 4.0.1, 4.1 Alpha2
> Reporter: Oleg Kalnichevski
> Fix For: 4.1 Alpha3
>
>
> IdleConnectionHandler when shutting down 'stale' connection does not update
> the state of AbstractPoolEntry thus causing an inconsistency between the
> state of the connection (closed) and that of the pool entry (still assumed
> open). The problem is mitigated by the fact that the pooling manager usually
> evicts closed connections almost immediately. There is a small window of time
> in the ThreadSafeClientConnManager#closeIdleConnection method, at which a
> connection can be closed by the IdleConnectionHandler and immediately leased
> from the pool by another thread in an inconsistent state before the main
> thread gets a chance to re-acquire the pool lock and clean out closed
> connections.
> For 4.0.x the problem can be worked around by retaining the pool lock for the
> entire span of the #closeIdleConnection. For the 4.1 branch a better solution
> should be devised. This probably means a complete rewrite or deprecation of
> IdleConnectionHandler.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
