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

Publisher#flatMapMerge allow terminal propagation after invalid demand #2348

Merged
merged 4 commits into from
Sep 12, 2022

Conversation

Scottmitch
Copy link
Member

Motivation:
Publisher#flatMapMerge has internal state that tracks demand. This state is used to drive the internal state machine and prevents invalid demand to avoid internal state being corrupted. However this code didn't always allow for the subsequent terminal (e.g. likely onError) to be propagated downstream which may result in a hang.

Modifications:

  • FlatMapPublisherSubscriber demand tracking shouldn't prevent terminal if there is too many items delivered.
  • Remove conditionals that are unnecessary for needsDemand because the call sites know if demand is required.

@Scottmitch Scottmitch marked this pull request as ready for review September 9, 2022 16:16
Motivation:
Publisher#flatMapMerge has internal state that tracks demand. This state
is used to drive the internal state machine and prevents invalid demand
to avoid internal state being corrupted. However this code didn't always
allow for the subsequent terminal (e.g. likely onError) to be propagated
downstream which may result in a hang.

Modifications:
- FlatMapPublisherSubscriber demand tracking shouldn't prevent terminal
  if there is too many items delivered.
- Remove conditionals that are unnecessary for needsDemand because the
  call sites know if demand is required.
@tkountis
Copy link
Contributor

Build failures due to #2245

@tkountis tkountis merged commit 2928a29 into apple:main Sep 12, 2022
@Scottmitch Scottmitch deleted the flatmap branch September 13, 2022 06:20
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