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

fix skip() race condition and request overflow #2987

Merged
merged 1 commit into from
May 28, 2015

Conversation

davidmoten
Copy link
Collaborator

OperatorSkip suffered from this race condition:

Suppose we wanted to skip 5 elements, then

  • two concurrent requests of say 1 between onStart request of 0 and first emission would request 1+ (5-0) and 1 + (5-0) elements = 12 elements. To deliver the two requested we only need 7 from upstream so we have overrequested.

It also lacked protection from request overflow.

Two unit tests have been added that failed on previous code and now pass.

@akarnokd
Copy link
Member

A shorter solution would be:

@Override
public void setProducer(Producer producer) {
    super.setProducer(producer);
    producer.request(toSkip);
}

which, in theory, should retain the ability to run unbounded and trigger the fast-paths.

@davidmoten
Copy link
Collaborator Author

Beautiful solution! I've updated the commit.

@akarnokd akarnokd added the Bug label May 28, 2015
@akarnokd
Copy link
Member

Sorry, I wasn't completely right and one slight change is required: replace super with child to avoid the possibility of interference from the Subscriber (because unspecified initial request is turned into MAX_VALUE request in some cases). The reactive-streams API is so much better/clearer in this regard...

@davidmoten
Copy link
Collaborator Author

righto, have done.

@akarnokd
Copy link
Member

Thanks!

akarnokd added a commit that referenced this pull request May 28, 2015
fix skip() race condition and request overflow
@akarnokd akarnokd merged commit ae09b86 into ReactiveX:1.x May 28, 2015
@benjchristensen
Copy link
Member

The reactive-streams API is so much better/clearer in this regard...

Yes it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants