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.

Sign up for GitHub

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

Jump to bottom

HTTP request may become non-retryable when first write fails #1644

Merged
merged 2 commits into from
Jul 2, 2021
Merged

HTTP request may become non-retryable when first write fails #1644

merged 2 commits into from
Jul 2, 2021

Conversation

idelpivnitskiy
Copy link
Member

@idelpivnitskiy idelpivnitskiy commented Jun 25, 2021 •
edited
Loading

Motivation:

Because WriteStreamSubscriber requests >1 items for new requests and
meta-data.concat(messageBody) subscribes to the messageBody asap,
it's not safe to retry these requests if the payload body does not
support multiple subscribes (non-replayable).

Modifications:

  • Reproduce described behavior in a test;
  • WriteStreamSubscriber requests only one item on the client-side and
    requests more only if the first write succeeds;
  • Use new operator Single.concat(Publisher, boolean) for
    AbstractStreamingHttpConnection and NettyHttpServer;
  • Remove unused requested counter from WriteStreamSubscriber;

Result:

Client subscribes to the message body only if the request meta-data is
written successfully. Otherwise, RetryableException is propagated.

@idelpivnitskiy idelpivnitskiy requested a review from chemicL July 1, 2021 18:06
@idelpivnitskiy
Copy link
Member Author

@chemicL I tried your suggestion to edit the PR and change the target branch to #1641, but it doesn't work because AbortedFirstWrite is in my fork, not in the main servicetalk repo. Please, use GH UI to select only the last commit for review: fbccead

chemicL
chemicL approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@chemicL chemicL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question, otherwise great job 👍

...ty-internal/src/main/java/io/servicetalk/transport/netty/internal/WriteStreamSubscriber.java
// subscription could be null if channelWritable is invoked before onSubscribe.
// If promise is not writable, then we will not be able to write anyways, so do not request more.
if (subscription == null || subscription == CANCELLED || !promise.isWritable()) {
return;
}

long n = demandEstimator.estimateRequestN(channel.bytesBeforeUnwritable());
long n = demandEstimator.estimateRequestN(bytesBeforeUnwritable >= 0 ? bytesBeforeUnwritable :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the check required? When bytesBeforeUnwritable is >= it's equal to channel.bytesBeforeUnwritable() (line 164), otherwise, channel.bytesBeforeUnwritable() is used directly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most frequent method that invokes requestMoreIfRequired is doWrite, where we already have capacityAfter. It's a way to avoid the second invocation of channel.bytesBeforeUnwritable() that will return the same result because we are on a single event-loop thread here. bytesBeforeUnwritable() is not super cheap because it does some math and volatile read internally.

@idelpivnitskiy
HTTP request may become non-retryable when first write fails
622b619 
Motivation:

Because `WriteStreamSubscriber` requests >1 items for new requests and
`meta-data.concat(messageBody)` subscribes to the `messageBody` asap,
it's not safe to retry these requests if the payload body does not
support multiple subscribes (non-replayable).

Modifications:

- Reproduce described behavior in a test;
- `WriteStreamSubscriber` requests only one item on the client-side and
requests more only if the first write succeeds;
- Use new operator `Single.concat(Publisher, boolean)` for
`AbstractStreamingHttpConnection` and `NettyHttpServer`;
- Remove unused `requested` counter from `WriteStreamSubscriber`;

Result:

Client subscribes to the message body only if request meta-data is
written successfully. Otherwise, `RetryableException` is propagated.
@idelpivnitskiy
Use promise.isWritable() instead of channel.isWritable()
2e595a8
@idelpivnitskiy idelpivnitskiy merged commit bfe3a34 into apple:main Jul 2, 2021
@idelpivnitskiy idelpivnitskiy deleted the retry-safely branch July 2, 2021 16:19
bondolo pushed a commit to bondolo/servicetalk that referenced this pull request Jul 2, 2021
@idelpivnitskiy @bondolo
HTTP request may become non-retryable when first write fails (apple#1644
94133a9 
)

Motivation:

Because `WriteStreamSubscriber` requests >1 items for new requests and
`meta-data.concat(messageBody)` subscribes to the `messageBody` asap,
it's not safe to retry these requests if the payload body does not
support multiple subscribes (non-replayable).

Modifications:

- Reproduce described behavior in a test;
- `WriteStreamSubscriber` requests only one item on the client-side and
requests more only if the first write succeeds;
- Use new operator `Single.concat(Publisher, boolean)` for
`AbstractStreamingHttpConnection` and `NettyHttpServer`;
- Remove unused `requested` counter from `WriteStreamSubscriber`;

Result:

Client subscribes to the message body only if the request meta-data is
written successfully. Otherwise, `RetryableException` is propagated.
bondolo pushed a commit to bondolo/servicetalk that referenced this pull request Jul 2, 2021
@idelpivnitskiy @bondolo
HTTP request may become non-retryable when first write fails (apple#1644
ac8937d 
)

Motivation:

Because `WriteStreamSubscriber` requests >1 items for new requests and
`meta-data.concat(messageBody)` subscribes to the `messageBody` asap,
it's not safe to retry these requests if the payload body does not
support multiple subscribes (non-replayable).

Modifications:

- Reproduce described behavior in a test;
- `WriteStreamSubscriber` requests only one item on the client-side and
requests more only if the first write succeeds;
- Use new operator `Single.concat(Publisher, boolean)` for
`AbstractStreamingHttpConnection` and `NettyHttpServer`;
- Remove unused `requested` counter from `WriteStreamSubscriber`;

Result:

Client subscribes to the message body only if the request meta-data is
written successfully. Otherwise, `RetryableException` is propagated.
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Jul 19, 2021
@idelpivnitskiy
Write of HTTP/2 message may never complete
759e39f 
Motivation:

In apple#1518 we optimized HTTP/2 to write less frames when it's known that
the message body is empty. However, that change and the apple#1644 follow-up
for `AbstractH2DuplexHandler` incorrectly handle end of outbound stream.
`AbstractH2DuplexHandler` writes the `endStream` flag and waits for an
empty trailers. Netty h2 codec will close the stream as soon as it sees
`endStream` for request and response and will invoke
`WriteStreamSubscriber#channelClosed(...)`. If offloading is enabled,
message body can be cancelled before trailers reached the flush
strategy. As the result, `flushOnEnd` will never be triggered and some
tests hang waiting for the response that will never be flushed. It also
affects tests related to `TransportObserver` because `writeComplete`
may never be triggered.

Modifications:

- Enhance `AbstractStreamingHttpConnection` and `NettyHttpServer` to
use only meta-data when it creates a flat stream of HTTP message if the
message body is empty;
- `AbstractH2DuplexHandler`: invoke
`CloseHandler#protocolPayloadEndOutbound(...)` before writing a frame
with `endStream` flag;
- Enhance tests for `AbstractH2DuplexHandler` to verify new behavior;

Result:

Fixes apple#1663.
@idelpivnitskiy idelpivnitskiy mentioned this pull request Jul 19, 2021
Merged
idelpivnitskiy added a commit that referenced this pull request Jul 20, 2021
@idelpivnitskiy
Write of HTTP/2 message may never complete (#1679)
339c847 
Motivation:

In #1518 we optimized HTTP/2 to write fewer frames when it's known that
the message body is empty. However, that change and the #1644 follow-up
for `AbstractH2DuplexHandler` incorrectly handle the end of outbound stream.
`AbstractH2DuplexHandler` writes the `endStream` flag and waits for
empty trailers. Netty h2 codec will close the stream as soon as it sees
`endStream` for request and response and will invoke
`WriteStreamSubscriber#channelClosed(...)`. If offloading is enabled,
the message body can be canceled before trailers reached the flush
strategy. As the result, `flushOnEnd` will never be triggered and some
tests hang waiting for the response that will never be flushed. It also
affects tests related to `TransportObserver` because `writeComplete`
may never be triggered.

Modifications:

- Enhance `AbstractStreamingHttpConnection` and `NettyHttpServer` to
use only meta-data when it creates a flat stream of HTTP message if the
message body is empty;
- `AbstractH2DuplexHandler`: invoke
`CloseHandler#protocolPayloadEndOutbound(...)` before writing a frame
with `endStream` flag;
- Enhance tests for `AbstractH2DuplexHandler` to verify new behavior;

Result:

Fixes #1663.
idelpivnitskiy added a commit that referenced this pull request Jul 21, 2021
@idelpivnitskiy
Write of HTTP/2 message may never complete (#1679)
4a829ec 
Motivation:

In #1518 we optimized HTTP/2 to write fewer frames when it's known that
the message body is empty. However, that change and the #1644 follow-up
for `AbstractH2DuplexHandler` incorrectly handle the end of outbound stream.
`AbstractH2DuplexHandler` writes the `endStream` flag and waits for
empty trailers. Netty h2 codec will close the stream as soon as it sees
`endStream` for request and response and will invoke
`WriteStreamSubscriber#channelClosed(...)`. If offloading is enabled,
the message body can be canceled before trailers reached the flush
strategy. As the result, `flushOnEnd` will never be triggered and some
tests hang waiting for the response that will never be flushed. It also
affects tests related to `TransportObserver` because `writeComplete`
may never be triggered.

Modifications:

- Enhance `AbstractStreamingHttpConnection` and `NettyHttpServer` to
use only meta-data when it creates a flat stream of HTTP message if the
message body is empty;
- `AbstractH2DuplexHandler`: invoke
`CloseHandler#protocolPayloadEndOutbound(...)` before writing a frame
with `endStream` flag;
- Enhance tests for `AbstractH2DuplexHandler` to verify new behavior;

Result:

Fixes #1663.
@idelpivnitskiy idelpivnitskiy mentioned this pull request Feb 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chemicL chemicL chemicL approved these changes

@bondolo bondolo Awaiting requested review from bondolo

@Scottmitch Scottmitch Awaiting requested review from Scottmitch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@idelpivnitskiy @chemicL