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

SplittingWriteEventsListener expects that an async source limits recursion depth to 1 #1652

Open
idelpivnitskiy opened this issue Jul 2, 2021 · 0 comments
Labels
bug Something isn't working

Comments

@idelpivnitskiy
Copy link
Member

idelpivnitskiy commented Jul 2, 2021

SplittingWriteEventsListener expects the following sequence of HTTP messages:

meta-data → Start  → Payload → InProgress → trailers → End

If the source does not limit recursion to a depth of 1 when it delivers data (see PublisherFrom2TckTest and PublisherFrom3TckTest), the sequence of events can be any, depending on how WriteDemandEstimator requests items.

Steps to reproduce:

  1. Change WriteDemandEstimators#newDefaultEstimator() to always return 1L from estimateRequestN.
  2. Run ContentLengthAndTrailersTest

By playing with WriteDemandEstimator you can achieve any sequence of events. Examples:

meta-data → trailers → End → Start
meta-data  → Payload → trailers → End  → InProgress → Start
meta-data  → Payload → InProgress → Start → Payload → trailers → End  → InProgress

Because the sequent can be any, SplittingWriteEventsListener and CountingFlushStrategyProvider can not correctly detect message boundaries. As the result, data may hang in the socket buffer and will never be flushed.

This is less problematic when task-based offloading is enabled because each onNext will be on a different task thread, but causes issues when offloading is disabled.

Workaround: flushOnEach() strategy, which will trigger the flush for any item, regardless of the boundary. Users can opt-out from our internal flush strategy optimizations by using streaming API or applying transform operation on the HTTP message in a filter (it will reset the "aggregated" and "empty" flags).

Ways to reduce the likelihood of this issue for flushOnEnd():

  • Limit recursion depth to 1 for all async sources and operators.

After this is fixed, revert the following patches:

@idelpivnitskiy idelpivnitskiy added the bug Something isn't working label Jul 2, 2021
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this issue Jul 2, 2021
Motivation:

To mitigate apple#1652 issue, temporary limit the mutual recursion between
`onNext` and `request` to a depth of 1.

Modifications:

- Use the left 4 bits of the `state` to limit recursion depth to 1;
- Remove `boundedDepthOfOnNextAndRequestRecursion()` override from
`PublisherFrom2TckTest` and `PublisherFrom3TckTest`;

Result:

1. `FromNPublisher` has maximum recursion depth of 1.
2. Less risk encountering the issue apple#1652.

Signed-off-by: Idel Pivnitskiy <[email protected]>
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this issue Jul 2, 2021
Motivation:

To mitigate apple#1652 issue, temporary limit the mutual recursion between
`onNext` and `request` to a depth of 1 for `ConcatDeferNextSubscriber`.

Modifications:

- Introduce additional `SINGLE_DELIVERING` state to understand if the
`request` was invoked while we deliver result of the `Single`;
- Remove `boundedDepthOfOnNextAndRequestRecursion()` override from
`SingleConcatWithPublisherDeferSubscribeTckTest`;

Result:

1. `SingleConcatWithPublisher` has maximum recursion depth of 1.
2. Less risk encountering the issue apple#1652.

Signed-off-by: Idel Pivnitskiy <[email protected]>
idelpivnitskiy added a commit that referenced this issue Jul 2, 2021
Motivation:

To mitigate #1652 issue, temporary limit the mutual recursion between
`onNext` and `request` to a depth of 1 for `ConcatDeferNextSubscriber`.

Modifications:

- Introduce additional `SINGLE_DELIVERING` state to understand if the
`request` was invoked while we deliver result of the `Single`;
- Remove `boundedDepthOfOnNextAndRequestRecursion()` override from
`SingleConcatWithPublisherDeferSubscribeTckTest`;

Result:

1. `SingleConcatWithPublisher` has a maximum recursion depth of 1.
2. Less risk of encountering issue #1652.
idelpivnitskiy added a commit that referenced this issue Jul 2, 2021
Motivation:

To mitigate #1652 issue, temporary limit the mutual recursion between
`onNext` and `request` to a depth of 1.

Modifications:

- Use the left 4 bits of the `state` to limit recursion depth to 1;
- Remove `boundedDepthOfOnNextAndRequestRecursion()` override from
`PublisherFrom2TckTest` and `PublisherFrom3TckTest`;

Result:

1. `FromNPublisher` has a maximum recursion depth of 1.
2. Less risk of encountering issue #1652.
bondolo pushed a commit to bondolo/servicetalk that referenced this issue Jul 2, 2021
Motivation:

To mitigate apple#1652 issue, temporary limit the mutual recursion between
`onNext` and `request` to a depth of 1 for `ConcatDeferNextSubscriber`.

Modifications:

- Introduce additional `SINGLE_DELIVERING` state to understand if the
`request` was invoked while we deliver result of the `Single`;
- Remove `boundedDepthOfOnNextAndRequestRecursion()` override from
`SingleConcatWithPublisherDeferSubscribeTckTest`;

Result:

1. `SingleConcatWithPublisher` has a maximum recursion depth of 1.
2. Less risk of encountering issue apple#1652.
bondolo pushed a commit to bondolo/servicetalk that referenced this issue Jul 2, 2021
Motivation:

To mitigate apple#1652 issue, temporary limit the mutual recursion between
`onNext` and `request` to a depth of 1.

Modifications:

- Use the left 4 bits of the `state` to limit recursion depth to 1;
- Remove `boundedDepthOfOnNextAndRequestRecursion()` override from
`PublisherFrom2TckTest` and `PublisherFrom3TckTest`;

Result:

1. `FromNPublisher` has a maximum recursion depth of 1.
2. Less risk of encountering issue apple#1652.
bondolo pushed a commit to bondolo/servicetalk that referenced this issue Jul 2, 2021
Motivation:

To mitigate apple#1652 issue, temporary limit the mutual recursion between
`onNext` and `request` to a depth of 1 for `ConcatDeferNextSubscriber`.

Modifications:

- Introduce additional `SINGLE_DELIVERING` state to understand if the
`request` was invoked while we deliver result of the `Single`;
- Remove `boundedDepthOfOnNextAndRequestRecursion()` override from
`SingleConcatWithPublisherDeferSubscribeTckTest`;

Result:

1. `SingleConcatWithPublisher` has a maximum recursion depth of 1.
2. Less risk of encountering issue apple#1652.
bondolo pushed a commit to bondolo/servicetalk that referenced this issue Jul 2, 2021
Motivation:

To mitigate apple#1652 issue, temporary limit the mutual recursion between
`onNext` and `request` to a depth of 1.

Modifications:

- Use the left 4 bits of the `state` to limit recursion depth to 1;
- Remove `boundedDepthOfOnNextAndRequestRecursion()` override from
`PublisherFrom2TckTest` and `PublisherFrom3TckTest`;

Result:

1. `FromNPublisher` has a maximum recursion depth of 1.
2. Less risk of encountering issue apple#1652.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant