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

Async fusion bug: extra fixes and changes #3046

Merged
merged 6 commits into from
Jun 7, 2022

Conversation

simonbasle
Copy link
Member

  • WIP try to fix other fusion issues, doFinally and doAfterTerminate not fuseable
  • fix hanging: StepVerifierBuilder cancels upstream AFTER propagating onError in ASYNC mode
  • remove doFinally fuseable-related unreachable code
  • fix tests: no fusion in doFinally, NONE negotiation in MonoPeekTerminal

@simonbasle simonbasle requested a review from a team as a code owner May 19, 2022 15:48
@simonbasle simonbasle marked this pull request as draft May 19, 2022 15:48
@simonbasle
Copy link
Member Author

the rationale for these changes: I started implementing an ArchUnit rule to surface other cases where a Subscriber is a QueueSubscription but doesn't implement onNext (and thus can break ASYNC fusion). It surfaced doFinally.

I tried to fix it and noticed that in the poll() path there was no catching of qs.poll() exceptions.
BUT unfortunately there is no clean way to implement fused doFinally/doAfterTerminate with thrown exceptions. For instance, assuming the following is fused:

 .doFinally(sig -> signals.add("finally"))
 .doOnError(e -> signals.add("onError"))

then the doFinally handler is invoked BEFORE the doOnError (which is contrary to the documented order and the unfused order).

On the other hand, this exploration also surfaced the fact that fused doOnError doesn't catch polling exceptions either, so the doOnError handler above is not even invoked. That one is fixable.

Lastly, in MonoPeekTerminal the afterTerminate handler is in the same situation as doFinally.
The easy solution implemented in this PR: remove Fuseable trait from doFinally and force fusion to be negotiated to NONE in case a MonoPeekTerminal has an afterTerminateHandler.

@simonbasle simonbasle added the type/bug A general bug label May 19, 2022
@simonbasle simonbasle added this to the 3.4.19 milestone May 19, 2022
Base automatically changed from 3044-doOnEachAsyncFusionBug to 3.4.x May 23, 2022 09:06
@simonbasle simonbasle marked this pull request as ready for review June 7, 2022 09:11
@simonbasle simonbasle force-pushed the 3044-extra-attemptToFixOtherOperators branch from d7cb912 to 65d3d2f Compare June 7, 2022 16:24
@simonbasle simonbasle merged commit 5d7a926 into 3.4.x Jun 7, 2022
@reactorbot
Copy link

@simonbasle this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to main 🙇

simonbasle added a commit that referenced this pull request Jun 7, 2022
@simonbasle simonbasle deleted the 3044-extra-attemptToFixOtherOperators branch June 7, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants