HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager #502
Conversation
| @@ -357,7 +357,19 @@ ManagedHttpClientConnection getConnection(final HttpRoute route, final Object st | |||
| this.conn = this.connFactory.createConnection(null); | |||
| this.created = System.currentTimeMillis(); | |||
| } else { | |||
| this.conn.activate(); | |||
| // Check if the existing connection is stale | |||
| if (this.conn.isStale()) { | |||
There was a problem hiding this comment.
@arturobernalg Should not the connection have already been checked for staleness in #validate line 355? Why doing again? Am I missing something?
There was a problem hiding this comment.
Should not the connection have already been checked for staleness in
#validateline 355? Why doing again? Am I missing something?
@ok2c
The staleness check was only being performed when the deadline had expired. In our case, the deadline wasn't expired, so the connection's staleness wasn't being evaluated.
There was a problem hiding this comment.
@arturobernalg Now I am confused. The #validate method performs the check only if the period of inactivity exceeds ValidateAfterInactivity setting. This is intentional and by design. One does not want to execute the stale check too often due to its cost and impact on performance. The change you are proposing seems to be breaking that. Is that really what you want? If one wants the stale check done every time one can set ValidateAfterInactivity to -1
There was a problem hiding this comment.
Upon reviewing your comment, I realize that I overlooked the intentional design behind the validate method's frequency of execution I've committed a new potential fix this. I have now submitted a new commit that aims to rectify this. The updated code ensures that the connection being released is indeed the one that was detached from the InternalConnectionEndpoint. This should maintain the intended behavior while addressing the issue at hand.
While I have conducted extensive testing to validate this fix, I must admit that connection management is not my primary area of expertise.
There was a problem hiding this comment.
@arturobernalg There is no problem of what so ever. That is why we have reviews
arturobernalg
force-pushed
the
HTTPCLIENT-2301
branch
2 times, most recently
from
70cf7d7
to
8c71081
Compare
| LOG.debug("{} Connection is not kept alive", id); | ||
| } | ||
| } else { | ||
| final boolean reusable = conn != null && conn.isOpen() && conn.isConsistent(); |
There was a problem hiding this comment.
@arturobernalg The variable seems redundant. Looks OK otherwise.
…This commit updates the release method to use the local conn variable from internalEndpoint.detach() for accurate state management and resource cleanup, addressing the issue HTTPCLIENT-2301.
arturobernalg
force-pushed
the
HTTPCLIENT-2301
branch
from
d2c1c8b
to
cb639a2
Compare
…This commit updates the release method to use the local conn variable from internalEndpoint.detach() for accurate state management and resource cleanup, addressing the issue HTTPCLIENT-2301. (#502)
The key change involves using the local
connvariable, obtained frominternalEndpoint.detach(), to ensure we're acting on the correct connection instance.