Skip to content

FluxReceive hangs, sometimes, on subsequent subscribers #503

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

Closed
rstoyanchev opened this issue Nov 8, 2018 · 5 comments
Closed

FluxReceive hangs, sometimes, on subsequent subscribers #503

rstoyanchev opened this issue Nov 8, 2018 · 5 comments
Labels
for/user-attention This issue needs user attention (feedback, rework, etc...) type/enhancement A general enhancement

Comments

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 8, 2018

FluxReceive rejects multiple subscribers but only if the second subscribes while first one is still active. Once a subscriber completes and the receiver field is cleared, a second subscriber will not be rejected and will not receive further signals.

One reason to read a second time is to ensure a client response has been drained from at a place where it isn't known if it has been consumed or not. The second attempt to read relies on either draining successfully or getting an error, not hanging indefinitely.

@smaldini smaldini added the type/enhancement A general enhancement label Nov 12, 2018
@violetagg violetagg added this to the 0.8.x Backlog milestone Nov 14, 2018
@rstoyanchev rstoyanchev changed the title FluxReceive to allow one subscriber only consistently FluxReceive hangs, sometimes, on subsequent subscriber Dec 5, 2018
@rstoyanchev rstoyanchev changed the title FluxReceive hangs, sometimes, on subsequent subscriber FluxReceive hangs, sometimes, on subsequent subscribers Dec 5, 2018
rstoyanchev added a commit to spring-projects/spring-framework that referenced this issue Dec 5, 2018
Commit #c187cb2 introduced proactive rejection of multiple subscribers
in ReactorClientHttpResponse, instead of hanging indefinitely as per
reactor/reactor-netty#503.

However FluxReceive also rejects subsequent subscribers if the response
is consumed fully, as opposed to being canceled, e.g. as with
bodyToMono(Void.class). In that case, a subsequent subscriber causes
two competing error signals to be sent, and one gets dropped and
logged by reactor-core.

This fix ensures that a rejection is raised in
ReactorClientHttpResponse only after a cancel() was detected.

Issue: SPR-17564
@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Dec 5, 2018

The following workaround was added to 5.1.3, which led to #540 and SPR-17564. It turns out the root cause was that both Reactor Netty and the Spring Framework were signalling IIlegalStateException on a subsequent subscriber, if the response was read in full by the first subscriber, but it works okay if the first subscriber cancels (e.g. response.bodyToMono(Void.class)), because in that case only the Spring Framework signals an error on subsequent subscribers. This was quite difficult to find out by the way. Here is the current behavior targetting 5.1.4.

The current behavior seems broken and our workaround proved fragile. I see no good reasons why subsequent subscribers, after a cancellation, should not be rejected as is the case with multiple subscribers during the course of reading. Even if there is some intent like retries, certainly indefinite hanging doesn't seem right either.

Once again the reason for talking about multiple subscribers in the first place is the need to ensure that response is drained always, and sometimes we can't be sure if it has been consumed or not. So we try to consume again and rely on being kicked out if it there has been a consumer already. If you'd like to keep the current behavior, then please consider exposing a way to do the equivalent of response.bodyToMono(Void.class). That's used where no response body is expected to ensure the body is drained, by attempting to read and cancel if any data shows up.

@smaldini smaldini modified the milestones: 0.8.x Backlog, 0.9.x Backlog Feb 7, 2019
@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Jun 23, 2020

@violetagg the following test here can be used to debug the issue. The scenario is this:

  1. An error response that is consumed with an onStatus handler which uses bodyToMono(Void.class) which does takeWhile and cancels if it finds any data.
  2. Cancellation leads to HttpClientOperations#onInboundCancel which closes the channel.
  3. A second subscription then from wrapper around the onStatus handler tries to consume again and ignore errors. That one goes to FluxReceiver#startReceiver but since at this point inboundDone is false it simply takes the subscription and nothing ever happens again.

I believe this behavior is somewhat intentional to keep the door open to allow re-subscribing but I'm not sure there is a clear purpose at this time. @smaldini any more insight?

@violetagg
Copy link
Member

@rstoyanchev This PR #1185 should fix the issue. Can you please verify it?

@violetagg violetagg modified the milestones: 0.9.x Maintenance Backlog, 0.9.10.RELEASE Jul 1, 2020
@violetagg violetagg added the for/user-attention This issue needs user attention (feedback, rework, etc...) label Jul 1, 2020
@violetagg
Copy link
Member

I'm closing this issue as a fix is provided with #1185

@rstoyanchev
Copy link
Contributor Author

The workaround has been removed in Spring Framework master and 5.2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for/user-attention This issue needs user attention (feedback, rework, etc...) type/enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants