Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset flush strategy after client request is written #3103

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

Currently we reset the flush strategy after a request is written and its response is received. If client starts writing another pipelined request on the same connection, it ends up using an overridden flush strategy of the previous request that is still waiting for its response.

Modifications:

  • For client connections (pipelined and non-pipelined) move flush strategy reset from "after full request-response completion" to "before completion of the request write". Using beforeFinally we guarantee that reset will happen before any retry/repeat operator initiates a new request on the same connection. It's safe to use "before" because for in-flight request the effective FlushStrategy is captured at the beginning of the write and doesn't change regardless of the reset. Only a new request will see the effect of the reset.
  • Reproduce described scenario in FlushStrategyOnClientTest.
  • Refactor FlushStrategyOnServerTest to use TransportObserver to observe flushes. This helps to use a real HttpServerContext instead of faking a ServerContext and reuse code for FlushStrategyOnClientTest.
  • Rename Cancellable in NettyHttpServer to highlight it's responsible for reset of FlushStrategy.

Result:

Second pipelined request on the same connection uses its own FlushStrategy.

Motivation:

Currently we reset the flush strategy after a request is written and its
response is received. If client starts writing another pipelined request
on the same connection, it ends up using an overridden flush strategy of
the previous request that is still waiting for its response.

Modifications:
- For client connections (pipelined and non-pipelined) move flush
strategy reset from "after full request-response completion" to
"before completion of the request write". Using `beforeFinally` we
guarantee that reset will happen before any retry/repeat operator
initiates a new request on the same connection. It's safe to use
"before" because for in-flight request the effective `FlushStrategy` is
captured at the beginning of the write and doesn't change regardless of
the reset. Only a new request will see the effect of the reset.
- Reproduce described scenario in `FlushStrategyOnClientTest`.
- Refactor `FlushStrategyOnServerTest` to use `TransportObserver` to
observe flushes. This helps to use a real `HttpServerContext` instead of
faking a `ServerContext` and reuse code for `FlushStrategyOnClientTest`.
- Rename `Cancellable` in `NettyHttpServer` to highlight it's
responsible for reset of FlushStrategy.

Result:

Second pipelined request on the same connection uses its own
`FlushStrategy`.
@idelpivnitskiy idelpivnitskiy merged commit 158d4c3 into apple:main Nov 15, 2024
11 checks passed
@idelpivnitskiy idelpivnitskiy deleted the flush branch November 15, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants