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

Operators: And, Then, When #506

Merged
merged 1 commit into from
Nov 22, 2013
Merged

Conversation

akarnokd
Copy link
Member

Issue #23, Issue #88, Issue #100

Can be extended to Plan4..Plan9 and Pattern4..Pattern9 if Action4..Action9 is available. Not sure about the ActionN version.

@cloudbees-pull-request-builder

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

@akarnokd
Copy link
Member Author

I don't understand. I used the master, haven't touched groupBy or the schedulers, the build succeeds on my machine (although with JDK 7). Now what?

Edit: I found the logical error in OperationGroupByTest:334; assuming the thread will finish if it emits 29-49 items is unreliable. Should that assertion be tested at all?

@zsxwing
Copy link
Member

zsxwing commented Nov 21, 2013

just another case of #383 (comment). The OperationGroupByTest test issue has not be fixed yet.

* Represents an activated plan.
*/
public abstract class ActivePlan0 {
protected final Map<JoinObserver, JoinObserver> joinObservers = new HashMap<JoinObserver, JoinObserver>();
Copy link
Member

Choose a reason for hiding this comment

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

Is a Plan ever accessed in a multi-threaded environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adapted the code from the latest Rx sources. Concurrency-related gate objects where present in the JoinObserver1 to protect a list of ActivePlan0 instances. The second place was in the when() to again build a list of ActivePlan0 instances. Since the Rx.NET source didn't contain any other locks, volatiles or concurrent instances, I assumed they are safe.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thanks for the validation.

@benjchristensen
Copy link
Member

That is an impressive contribution @akarnokd. I don't see any problems in my review. The unit tests were essential to understanding and trusting this, so thank you for being thorough.

I'm going to merge this and leverage the fact that we're still pre 1.0 so that if there are any issues found we can still fix them even if we need to break a signature.

benjchristensen added a commit that referenced this pull request Nov 22, 2013
@benjchristensen benjchristensen merged commit d511fa3 into ReactiveX:master Nov 22, 2013
rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
@akarnokd akarnokd deleted the AndPattern2 branch January 13, 2014 10:05
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.

None yet

4 participants