1.x: implement OperatorDoOnEmpty, with Observable.doOnEmpty() operator by thomasnield · Pull Request #3624 · ReactiveX/RxJava

thomasnield · 2016-01-19T02:48:50Z

davidmoten · 2016-01-19T06:54:03Z

Should this call get a try catch and Exceptions.throwOrReport called? If so, I'll put in a PR for OperatorDoAfterTerminate as well.

I might have misunderstood Exceptions.throwOrReport. Does "reporting" pass it up the chain via onError() and throw simply throws an exception without passing it?

thomasnield · 2016-01-19T17:47:51Z

I saw there was an OperatorDoOnEach which was being paired with an Observer, so I went ahead and scrapped OperatorDoOnEmpty in favor of using that pattern.

thomasnield · 2016-01-19T17:57:15Z

Okay two questions:

What exactly am I testing for with unsubscription? That Subscription.isUnsubscribed() is true after calling Subscription.unsubscribe()? That items are no longer emitting? Do I have to test for hot and cold sources?
Can you point me to another unit test that does the backpressure test you have in mind?

akarnokd · 2016-01-19T20:53:09Z

Please make sure the imports stay with * because it is a source of merge conflict otherwise.

akarnokd · 2016-01-19T20:56:53Z

unsubscription test:

use a PublishSubject, this operator and take, push through values up to the limit and check hasObservers on the subject

backpressure

use a range, this operator and subscribe a TestSubscriber which has been created with 0 as an initial value. Use requestMore(1) and assert the testsubscriber received only a single value, no error and no completion

thomasnield · 2016-01-20T00:06:49Z

@akarnokd Done. The unsubscription test made sense to me. Not too familiar with the TestSubscriber. Regardless, I implemented what I think you meant and the tests passed.

thomasnield · 2016-01-20T00:18:49Z

@akarnokd Might help if I actually used the operator in question. One second...

thomasnield · 2016-01-20T00:20:46Z

Okay unit tests are complete.

davidmoten · 2016-01-20T00:28:49Z

Hey Thomas, for your convenience what you normally do is make a branch of 1.x say doOnEmpty, do your change in that branch and submit your PR from that branch rather than from 1.x. This PR doesn't have to be changed though.

thomasnield · 2016-01-20T00:29:42Z

Okay, thanks @davidmoten. I'll branch the feature separately next time.

davidmoten · 2016-01-20T00:33:30Z

Should use assertTrue everywhere in the unit tests rather than assert to ensure the assertions are run regardless of runtime flags.

thomasnield · 2016-01-20T00:43:09Z

Dang it IDEA, sorry my habits have caused Observable to have irrelevant changes. One moment.

akarnokd · 2016-01-20T08:20:50Z

There are too many, probably irrelevant, changes in Observable.java. Could you start from a clean file?

In addition, your reuse of doOnEach may not work because were sharing the empty flag across subscriptions whose source may not be always empty. For example,

Observable<Integer> o = Observable.defer(
    () -> Observable.range(1, counter.getAndIncrement() % 2))
.doOnEmpty(() -> System.out.println("Empty!"));

o.subscribe(System.out::println);
o.subscribe(System.out::println);
o.subscribe(System.out::println);
o.subscribe(System.out::println);
o.subscribe(System.out::println);

This should print 3 Empty! but only prints 1.

thomasnield · 2016-01-20T16:49:45Z

That seems like a deliberate side effect almost... so what would the expected behavior be? Because I don't really see how it is much different from onCompleted() other than it has a condition before performing the action. I'll try to reason through what you're showing...

thomasnield · 2016-01-20T16:53:14Z

Oh shoot, I think I see what you are saying. I was sharing state between the subscriptions. Let me figure out how to do this...

thomasnield · 2016-01-20T18:37:11Z

Alright, I started over on the implementation and placed that Observable.defer() case as an additional test. All five unit tests passed.

akarnokd · 2016-01-20T19:02:07Z

Please use assertFalse

You forgot one assertTrue.

davidmoten · 2016-02-09T05:01:31Z

+                    try {
+                        onEmpty.call();
+                    } catch (Exception e) {
+                        Exceptions.throwIfFatal(e);


If a non-fatal error occurs in onEmpty.call() then at the moment it would complete normally. Moreover you should catch for Throwables not just Exceptions.

This should be

catch (Throwable e) { Exceptions.throwOrReport(e, this); return; }

Tests for these scenarios would be nice too.

Okay I will put all these on my list today.

thomasnield · 2016-02-09T19:58:52Z

Question. Why is it the policy that doOnXXX() operators won't emit an exception that occurs in the supplied side effect function? Is the idea that it is up to the client to deal with the exception that occurs in _their _function since it is after all a side-effect? And it is not part of the Observable?

EDIT

Let me rephrase that. Should exceptions in the side-effect function be emitted to the child's onError()? Because I don't think I'm doing that here...

thomasnield · 2016-02-09T20:12:56Z

+                    try {
+                        onEmpty.call();
+                    } catch (Throwable e) {
+                        Exceptions.throwIfFatal(e);


Is this the proper way to handle an error occurring in the side effect function onEmpty?

No Thomas, I've given you the way to do it in my previous comment (and the method used calls onError).

Okay I thought I missed something in my notes. I'll fix it.

Exceptions.throwOrReport(), that's what I was looking for (and I think that is what you said). Thanks for your patience, I'm learning a lot.

davidmoten · 2016-02-10T05:03:54Z

+                        onEmpty.call();
+                    } catch (Throwable e) {
+                        Exceptions.throwOrReport(e,this);
+                    }


add return; after call to Exceptions.throwOrReport. Thomas I suggest you read the comments on the github site because it seems like you are reacting to truncated comments (perhaps email notifications).

You're right I was reading email notifications.

davidmoten · 2016-03-03T01:25:25Z

+                }
+                child.onCompleted();
+                done = true;
+            }


Because onEmpty.call could take a non-trivial time to complete I would normally put in a isUnsubscribed check before calling child.onCompleted(). I'd suggest this rejig:

@Override public void onCompleted() { if (done) { return; } if (isEmpty) { try { onEmpty.call(); } catch (Throwable e) { Exceptions.throwOrReport(e,this); return; } if (!isUnsubscribed()) { child.onCompleted(); } } else { child.onCompleted(); } done = true; }

thomasnield · 2016-03-03T07:02:35Z

Okay it's done, @davidmoten. Please let me know if I missed anything else...

thomasnield · 2016-03-03T07:11:51Z

It looks like a test failed. I don't think it has to do with this feature, but I'll confirm tomorrow after I update my fork and see if that rids it.

davidmoten · 2016-06-29T11:04:15Z

I think this operator was about done so I chucked it into https://github.com/davidmoten/rxjava-extras. That ok with you @thomasnield?

thomasnield · 2016-06-29T11:59:20Z

@davidmoten That sounds great! I'm glad you did, thanks!

thomasnield mentioned this pull request Jan 19, 2016

doOnEmpty() Operator? #3621

Closed

davidmoten reviewed Jan 19, 2016
View reviewed changes

akarnokd added the Feature-Request label Jan 19, 2016

akarnokd reviewed Jan 19, 2016
View reviewed changes

davidmoten reviewed Jan 20, 2016
View reviewed changes

Implement OperatorDoOnEmpty

f07de13

restore imports

42cce06

akarnokd reviewed Jan 20, 2016
View reviewed changes

thomasnield added 2 commits January 20, 2016 13:22

remove IDEA formatting

bc5df50

put doOnEmpty back

0b240aa

akarnokd changed the title ~~implement OperatorDoOnEmpty, with Observable.doOnEmpty() operator~~ 1.x: implement OperatorDoOnEmpty, with Observable.doOnEmpty() operator Jan 22, 2016

DavidMGross mentioned this pull request Jan 22, 2016

Document new doOnEmpty() operator ReactiveX/reactivex.github.io#183

Closed

use assertFalse

605d4d1

davidmoten reviewed Feb 9, 2016
View reviewed changes

thomasnield added 2 commits February 9, 2016 09:25

Implement Throwable, not Exception

4150aab

Forgot an assertFalse

4f8fa97

thomasnield reviewed Feb 9, 2016
View reviewed changes

use Exceptions.throwOrReport() for OperatorDoOnEmpty

d0f95c2

davidmoten reviewed Feb 10, 2016
View reviewed changes

Put return after onError() call

02ffb8c

davidmoten reviewed Mar 3, 2016
View reviewed changes

Unsubscription and fixes for doOnEmpty()

70372f2

thomasnield closed this Apr 14, 2016

davidmoten mentioned this pull request Jun 30, 2016

doOnEmpty implementation davidmoten/rxjava-extras#16

Open

Conversation

thomasnield commented Jan 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasnield commented Jan 19, 2016

Uh oh!

thomasnield commented Jan 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akarnokd commented Jan 19, 2016

Uh oh!

thomasnield commented Jan 20, 2016

Uh oh!

thomasnield commented Jan 20, 2016

Uh oh!

thomasnield commented Jan 20, 2016

Uh oh!

davidmoten commented Jan 20, 2016

Uh oh!

thomasnield commented Jan 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasnield commented Jan 20, 2016

Uh oh!

akarnokd commented Jan 20, 2016

Uh oh!

thomasnield commented Jan 20, 2016

Uh oh!

thomasnield commented Jan 20, 2016

Uh oh!

thomasnield commented Jan 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasnield commented Feb 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasnield commented Mar 3, 2016

Uh oh!

thomasnield commented Mar 3, 2016

Uh oh!

davidmoten commented Jun 29, 2016

Uh oh!

thomasnield commented Jun 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants