Publisher#flatMapConcatIterable may skip emitting items #3108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Scottmitch
force-pushed
the
pub_concat_map_iter
branch
6 times, most recently
from
b462578
to
831bf48
Compare
Motivation: Publisher#flatMapConcatIterable may not emit some items due to race conditions and visibility issues. The iterator state is written to outside the scope of holding the lock. After a drain loop completes we may request 1 more iterator. However it is possible the thread emitting holds the lock while another thread invokes onNext(t). The emitting thread may not see the iterator, and instead see `EmptyIterator.instance()` and cause it to request 1 more item, but then the not-visible iterator contents won't be emitted. Modifications: - Make FlatMapIterableSubscriber iterator state volatile and atomically update it. There is only ever 1 valid iterator because only 1 outstanding demand is issued only after the current iterator `!hasNext()`. The iterator state is re-read on each drain loop, and the terminal condition must atomically set to EmptyIterator.
Scottmitch
force-pushed
the
pub_concat_map_iter
branch
from
831bf48
to
98c817a
Compare
|
lgtm as well. Nice find. |
|
How did we find this? Was it reported by a user? |
|
@mgodave yes, and that user provided reproducer |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Publisher#flatMapConcatIterable may not emit some items due to race conditions and visibility issues. The iterator state is written to outside the scope of holding the lock. After a drain loop completes we may request 1 more iterator. However it is possible the thread emitting holds the lock while another thread invokes onNext(t). The emitting thread may not see the iterator, and instead see
EmptyIterator.instance()and cause it to request 1 more item, but then the not-visible iterator contents won't be emitted.Modifications:
!hasNext(). The iterator state is re-read on each drain loop, and the terminal condition must atomically set to EmptyIterator.