[jira] Commented: (HTTPCLIENT-948) IdleConnectionHandler can leave closed connections in a inconsistent state

["Sam Berlin (JIRA)" <ji...@apache.org>]

    [ 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: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org