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

Avoid deadlock consuming from blocking Iterator #2204

Merged
merged 2 commits into from
May 6, 2022

Conversation

Scottmitch
Copy link
Member

Motivation:
The blocking streaming APIs allow for specifying a Iterator
as input which is consumed by ServiceTalk. ServiceTalk assumes
it can consume in batches from the same thread in order to make
progress. However if the Iterator next() method is dependent
on external input we may deadlock:

  • PublisherAsBlockingIterable.onSubscribe(..) does a request(16)
  • FromIterablePublisher.request(..) loops calling hasNext() and
    next() until demand is exhausted. Each loop iteration calls
    subscriber.onNext(next()).
  • The same thread looping in FromIterablePublisher.request(..) is
    also responsible for draining the queue in PublisherAsBlockingIterable.
    This means that no data will be sent downstream until
    FromIterablePublisher exits the loop. This maybe perceived as
    “dead lock” because no data is sent even if the Iterable provides an
    element.

Modifications:

  • FromIterablePublisher and FromBlockingIterablePublisher check to
    unwrap from PublisherAsBlockingIterable if possible to directly
    access the underlying Publisher. This skips thee consumption
    via thee Iterable API and avoids the dead lock loop.

Result:
No more dead lock when using HTTP / gRPC API conversions and providing
Iterable that is dependent upon external events to make progress.

Motivation:
The blocking streaming APIs allow for specifying a Iterator
as input which is consumed by ServiceTalk. ServiceTalk assumes
it can consume in batches from the same thread in order to make
progress. However if the Iterator `next()` method is dependent
on external input we may deadlock:
- `PublisherAsBlockingIterable.onSubscribe(..)` does a `request(16)`
- `FromIterablePublisher.request(..)` loops calling `hasNext()` and
  `next()` until demand is exhausted. Each loop iteration calls
  `subscriber.onNext(next())`.
- The same thread looping in `FromIterablePublisher.request(..)` is
  also responsible for draining the queue in `PublisherAsBlockingIterable`.
  This means that no data will be sent downstream until
  `FromIterablePublisher` exits the loop. This maybe perceived as
  “dead lock” because no data is sent even if the `Iterable` provides an
  element.

Modifications:
- `FromIterablePublisher` and `FromBlockingIterablePublisher` check to
   unwrap from `PublisherAsBlockingIterable` if possible to directly
   access the underlying `Publisher`. This skips thee consumption
   via thee `Iterable` API and avoids the dead lock loop.

Result:
No more dead lock when using HTTP / gRPC API conversions and providing
`Iterable` that is dependent upon external events to make progress.
@Scottmitch
Copy link
Member Author

followup investigation required: #2205, however I would like to unblock the immediate concern.

@Scottmitch Scottmitch merged commit 9aec0e0 into apple:main May 6, 2022
@Scottmitch Scottmitch deleted the block_stream_dead branch May 6, 2022 21:35
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.

3 participants