#629 Error strategy support for handle. #957
Conversation
|
@simonbasle I've added a ton of tests for |
| else { | ||
| reset(); | ||
| s.request(1L); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think it would need the s.cancel after this block (from original line 109)
There was a problem hiding this comment.
Cancellation happens in Operators.onNextError if the error is not handled, so operator itself can't cancel prior to invoking strategy.
There was a problem hiding this comment.
the old code would always cancel s, whereas with your modification it only cancels when there is an error without continue mode, not when there is completion.
| } | ||
| else { | ||
| reset(); | ||
| s.request(1); |
There was a problem hiding this comment.
shouldn't be needed in tryOnNext, the return false will be interpreted as "request more"
There was a problem hiding this comment.
This is onNext() though.
| } | ||
| } | ||
| else { | ||
| actual.onComplete(); |
There was a problem hiding this comment.
need to do the s.cancel() in this block
There was a problem hiding this comment.
Cancellation on completion?
There was a problem hiding this comment.
the old code would always cancel s, whereas with your modification it only cancels when there is an error without continue mode, not when there is completion.
| else { | ||
| done = false; | ||
| error = null; | ||
| s.request(1); |
There was a problem hiding this comment.
replace with a return false
| } | ||
| else { | ||
| done = false; | ||
| error = null; |
There was a problem hiding this comment.
this.error is hidden by local error variable that you used here. need to replace with this.error or use a reset() method like you did so far
| throw Exceptions.propagate(e_); | ||
| } | ||
| else { | ||
| done = false; |
There was a problem hiding this comment.
same as above, use reset() to avoid wrong variable being reset
| throw Exceptions.propagate(e_); | ||
| } | ||
| else { | ||
| done = false; |
dfeist
force-pushed
the
629-errorMode
branch
from
8a25305 to
019ab70
Compare
dfeist
force-pushed
the
629-errorMode
branch
from
29b98f5 to
d09306e
Compare
simonbasle
dismissed
their stale review
need to look again at if(done) cases
| } | ||
| return false; |
There was a problem hiding this comment.
not sure about this one. processing the error and terminating was considered as a consumption of the tryOnNext so I think it was the right thing to do to only return false when there's a continue.
There was a problem hiding this comment.
This was already returning false for the onError (no failure handling) case, as does FluxFilter, so no change there. And of course, the continue case should return false, and does.
There was a problem hiding this comment.
wouldn't it previously fall through to return true though?
https://github.com/reactor/reactor-core/blob/master/reactor-core/src/main/java/reactor/core/publisher/FluxHandleFuseable.java#L124
There was a problem hiding this comment.
Take a look at https://github.com/reactor/reactor-core/blob/master/reactor-core/src/main/java/reactor/core/publisher/FluxFilter.java#L116, this is what I was basing things on. Maybe this is wrong, but all FluxFilter tryOnNext paths use false return in case of error with default stop strategy.
There was a problem hiding this comment.
FluxMap on the other hand returns true in this case. Wondering if this difference is deliberate or it's an inconsistency :-( Any idea?
dfeist
force-pushed
the
629-errorMode
branch
from
654a83f to
df599ff
Compare
…ilter might be wrong unless I'm missing something)
dfeist
force-pushed
the
629-errorMode
branch
from
22011e0 to
941a2ed
Compare
see also discussion in PR #957
see also discussion in PR #957
see also discussion in PR reactor#957
see also discussion in PR #957
see also discussion in PR #957
No description provided.