Reduce probability of LB selecting an unusable connection #948
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
__Motivation__ `NettyChannelPublisher` closes the connection if it sees a `cancel()` before all data for the current `Subscriber` is read. However, the client layer makes the connection eligible for selection by load balancer if it sees a `cancel()` being unaware that the connection MAY be eventually closed by the transport. This creates a race condition where load balancer may select a connection which is going to be closed. __Modification__ Pessimistically assume that if we see a `cancel()` for a request, transport will close the connection so force close the connection. This is pessimistic because it may so happen that completion of read is racing with the cancel and transport may see read completion before cancel and hence not close the connection. At the client layer (where we control LB selection eligibility), it is impossible to discern whether transport will close the connection or not so the safest option is to assume closure. This reduces the possibility of selecting an unusable connection hence the pains associated with debugging those situations at the cost of closing the connection. For H2, this will be a stream, which is cheap to close but for H1 this will close the actual connection. __Result__ Reduce possibility of selecting an unusable connection. As `cancel()` is not the only reason for closure, we still have the case where a connection can be selected only to be closed later but we reduce that probability with this change.
NiteshKant
commented
Feb 25, 2020
| import static io.servicetalk.concurrent.api.SourceAdapters.toSource; | ||
| import static io.servicetalk.http.api.HttpEventKey.MAX_CONCURRENCY; | ||
|
|
||
| final class ConcurrentRequestsHttpConnectionFilter implements FilterableStreamingHttpConnection { |
There was a problem hiding this comment.
This was dead code.
idelpivnitskiy
approved these changes
Feb 25, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
NettyChannelPublishercloses the connection if it sees acancel()before all data for the currentSubscriberis read. However, the client layer makes the connection eligible for selection by load balancer if it sees acancel()being unaware that the connection MAY be eventually closed by the transport. This creates a race condition where load balancer may select a connection which is going to be closed.Modification
Pessimistically assume that if we see a
cancel()for a request, transport will close the connection so force close the connection. This is pessimistic because it may so happen that completion of read is racing with the cancel and transport may see read completion before cancel and hence not close the connection. At the client layer (where we control LB selection eligibility), it is impossible to discern whether transport will close the connection or not so the safest option is to assume closure. This reduces the possibility of selecting an unusable connection hence the pains associated with debugging those situations at the cost of closing the connection.For H2, this will be a stream, which is cheap to close but for H1 this will close the actual connection.
Result
Reduce possibility of selecting an unusable connection. As
cancel()is not the only reason for closure, we still have the case where a connection can be selected only to be closed later but we reduce that probability with this change.