Repository navigation
Improved handling of idle sessions by AbstractIOSessionPool / H2ConnPool - #715
Conversation
arturobernalg
left a comment
arturobernalg
left a comment
There was a problem hiding this comment.
@ok2c IOSession#getLastEventTime() is based on System.nanoTime(), while inactivityDeadline() is based on Clock#millis() converted to nanoseconds. These use different time origins, so I don't think they can be compared directly. Am I missing something?
|
@arturobernalg I suspected something was off. I misunderstood how |
86eee99 to
e1800cd
Compare
|
@arturobernalg Please do another pass |
| protected boolean isIdle(final IOSession session) { | ||
| final IOEventHandler handler = session.getHandler(); | ||
| if (handler instanceof HttpConnection) { | ||
| return ((HttpConnection) handler).isIdle(); |
There was a problem hiding this comment.
Could this condition be inverted? Shouldn’t isIdle() return true when streamCount() == 0 rather than > 0?
| super(clock); | ||
| this.connectionInitiator = Args.notNull(connectionInitiator, "Connection initiator"); | ||
| this.addressResolver = addressResolver != null ? addressResolver : DefaultAddressResolver.INSTANCE; | ||
| this.addressResolver = addressResolver; |
There was a problem hiding this comment.
Was the change in the addressResolver fallback intentional? With a null resolver this no longer seems equivalent to DefaultAddressResolver, in particular for default ports and an explicitly provided HttpHost address.
There was a problem hiding this comment.
@arturobernalg It was. H2ConnPool violates our package layering policy by importing an impl class into a non-impl one. However you likely have more violations like that. They all should be addressed consistency across teh entire code base.
I reverted my changes.
0e67a26 to
a161925
Compare
a161925 to
72cc85a
Compare
2 participants
@arturobernalg you have been working a lot with the connection pools. Could you please double-check my changes to make sure we are on the same page as far as idle connection handling is concerned?