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

Rework FluxReplay to avoid hanging, but reject 0 size #2741

Merged
merged 9 commits into from Sep 9, 2021
Merged

Conversation

OlegDokuka
Copy link
Contributor

closes #1921

This PR adds a prefetch strategy driven by active subscribers so the next prefetch happens only when all the subscribers have consumed a particular element. To enable that, now every impl has an index actively utilized to identify if specific element is a trigger for the next prefetch.

In case there are no subscribers, the prefetch strategy is self-driven and requests more when there are produced N elements into the cache

Signed-off-by: Oleh Dokuka odokuka@vmware.com

@OlegDokuka OlegDokuka added type/bug A general bug type/enhancement A general enhancement labels Jul 14, 2021
@OlegDokuka OlegDokuka requested a review from a team as a code owner July 14, 2021 11:42
@OlegDokuka OlegDokuka marked this pull request as draft July 14, 2021 11:42
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@@ -148,9 +159,10 @@
long maxAge,
Scheduler scheduler) {
this.limit = limit;
this.indexUpdateLimit = Operators.unboundedOrLimit(limit);
Copy link
Member

Choose a reason for hiding this comment

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

as demonstrated by (currently failing) reactor.core.publisher.SinksTest.MulticastReplayN#checkSemanticsSize0, limit can be zero here which would lead to ArithmeticException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove tests that check zero history size and added protection against passing zero as a history size since it is meaning-less for replay operator (in such scenario we can suggest alternative represented in the .publish operator)

Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka OlegDokuka marked this pull request as ready for review July 30, 2021 11:17
@OlegDokuka
Copy link
Contributor Author

@simonbasle most of the things were fixed / improved. If all good, will be adding stresstests

Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

a small clarification on javadoc wording still, but otherwise looks good.
the commit message will need to be explicit that this commit removes support for historySize(0), just in case some users weirdly depend on it.

go ahead with stress tests

@@ -1598,7 +1606,7 @@ static long markDisposed(ReplaySubscriber<?> instance) {
}

/**
* Check if state has subscribed flag indicating subscription reception
* Check if state has {@link #CONNECTED_FLAG} flag indicating subscription reception
Copy link
Member

Choose a reason for hiding this comment

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

which of SUBSCRIBED_FLAG and CONNECTED_FLAG indicates subscription reception? both this javadoc and the one below now correctly link to the right constant, but they otherwise use the exact same phrasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@simonbasle
Copy link
Member

For the pragmatic pov of not breaking anybody that would depend on history size 0 in a patch release (probably by mistake, but still...) what do you think about the operator method delegating to Flux.cache when size is zero @OlegDokuka?

@OlegDokuka
Copy link
Contributor Author

@sbasleTo be BC I would replace that automatically with with Flux Publish

@simonbasle
Copy link
Member

simonbasle commented Aug 5, 2021

I would replace that automatically with with Flux Publish

Yeah I think that's what I meant: Flux#replay(int size) with size == 0 would delegate to Flux#publish() (indeed, since it returns a ConnectableFlux while cache returns a Flux)

There could also be a warning logged, or a mention in the release notes that next major version will reject 0 parameter.

@simonbasle simonbasle added this to the 3.4.10 milestone Aug 9, 2021
@simonbasle simonbasle linked an issue Aug 9, 2021 that may be closed by this pull request
@simonbasle simonbasle added warn/behavior-change Breaking change of publicly advertised behavior and removed type/enhancement A general enhancement labels Sep 9, 2021
@simonbasle
Copy link
Member

This is not entirely ideal, but we're making the bet here that nobody actually uses historySize == 0 in the Sinks side of things, since it is a younger API.

Still marking as warn, because a 0 value is now actively rejected by an IllegalArgumentException.

For Flux.replay(historySize), we're a bit more conservative and continue allowing that usecase by transparently switching to a FluxPublish implementation rather than FluxReplay. A future version should probably also explicitly reject such calls, however.

@simonbasle simonbasle changed the title reworks FluxReplay to follow prefetch strategy Rework FluxReplay to avoid hanging, but reject 0 size Sep 9, 2021
@simonbasle simonbasle merged commit a596764 into main Sep 9, 2021
@simonbasle simonbasle deleted the bugfix/replay branch September 9, 2021 16:46
@osi
Copy link
Contributor

osi commented Sep 14, 2021

This is not entirely ideal, but we're making the bet here that nobody actually uses historySize == 0 in the Sinks side of things, since it is a younger API.

While I don't remember why I had this construct, I do have a usage of

Sinks.many().replay().limit(0)

within my tests which failed when ingesting the dependency update with this change.

(I suspect I wanted a sink that would allow multiple subscriptions and replay terminal signals)

@osi
Copy link
Contributor

osi commented Sep 15, 2021

How can I create a sink that:

  • Does not fail for emissions with zero subscribers
  • Allows resubscription

Using Sinks.many().replay().limit(0) did this.

None of the multicast sinks work, as one can't set the backpressure buffer size to 0. The direct* versions fail when there are no subscribers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug warn/behavior-change Breaking change of publicly advertised behavior
Projects
None yet
3 participants