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 issue #1522 #1523

Merged
merged 3 commits into from
Jul 29, 2014
Merged

Fix issue #1522 #1523

merged 3 commits into from
Jul 29, 2014

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Jul 26, 2014

TakeLast should ignore other requests if it's requested with Long.MAX_VALUE. #1522

@cloudbees-pull-request-builder

RxJava-pull-requests #1441 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #1442 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #1443 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #1445 SUCCESS
This pull request looks good

// Therefore, always check before setting to `c+n`
return;
}
if (REQUESTED_UPDATER.compareAndSet(this, _c, _c + n)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can still just use getAndAdd without the CAS retry loop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I try to ignore the further requests after request(Long.MAX_VALUE). I cannot use getAndAdd because if requested == Long.MAX_VALUE, I wanna ignore n. If adding n to requested, and if it happens to execute in while (true) loop in emit method, the loop won't stop because requested < 0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the overflow, I mean sending a request(n) after request(Long.MAX_VALUE) may make requested overflow easily. c+n is rarely overflow if request != Long.MAX_VALUE (However, we still can handle c+n overflow problem by adding a check c > Long.MAX_VALUE - n).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloudbees-pull-request-builder

RxJava-pull-requests #1450 FAILURE
Looks like there's a problem with this pull request

Signed-off-by: zsxwing <zsxwing@gmail.com>
@cloudbees-pull-request-builder

RxJava-pull-requests #1453 SUCCESS
This pull request looks good

@@ -60,6 +60,9 @@ public void onNext(T value) {
} else {
this.value = value;
isNonEmpty = true;
// Issue: https://github.com/Netflix/RxJava/pull/1527
// Because we cache a value and don't emit now, we need to request another one.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

@benjchristensen
Copy link
Member

I think this looks good, though I'm still curious if the complexity here is worth us having a fast-path. Would be interesting to do JMH testing. In OnSubscribeFromRange it does make a difference, but here I question it because every onNext is passing through a Queue.

benjchristensen added a commit that referenced this pull request Jul 29, 2014
@benjchristensen benjchristensen merged commit 400a611 into ReactiveX:master Jul 29, 2014
@zsxwing zsxwing deleted the issue-1522 branch August 2, 2014 14:11
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