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

Clarification on multiple subscribers needed .. #284

Closed
RuedigerMoeller opened this Issue Jul 3, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@RuedigerMoeller
Copy link

RuedigerMoeller commented Jul 3, 2015

I implemented multiple subscribers using a "wait-for-slowest-subscriber" policy. This means if of 2 subscribers only one sends "credits" via request(N), no message will be sent. So credits = Min(credits of subscribers)

this makes

required_mustRequestFromUpstreamForElementsThatHaveBeenRequestedLongAgo()
and required_spec104_mustCallOnErrorOnAllItsSubscribersIfItEncountersANonRecoverableError() fail.

But "wait-for-slowest-subscriber" (with optional timout/forced cancel policy) actually does not violate the spec, isn't it ?

@RuedigerMoeller RuedigerMoeller changed the title Clarification on mutliple subscribers needed .. Clarification on multiple subscribers needed .. Jul 3, 2015

@ktoso

This comment has been minimized.

Copy link
Contributor

ktoso commented Jul 4, 2015

Hi @RuedigerMoeller, thanks a lot for catching this - you're absolutely correct.
These tests assume too much about the Publisher - namely that it just signals the elements once demand comes in. I've skimmed the impls now and it seems that amending them won't be too hard - I'll send in a PR soon to fix this.

@RuedigerMoeller

This comment has been minimized.

Copy link
Author

RuedigerMoeller commented Jul 4, 2015

Thanks for the quick response :). Kudos for the test suite, it discovered an awful lot of issues in my initial implementation ..

ktoso added a commit to ktoso/reactive-streams that referenced this issue Jul 12, 2015

ktoso added a commit to ktoso/reactive-streams that referenced this issue Jul 12, 2015

ktoso added a commit to ktoso/reactive-streams that referenced this issue Jul 12, 2015

ktoso added a commit to ktoso/reactive-streams that referenced this issue Jul 12, 2015

ktoso added a commit to ktoso/reactive-streams that referenced this issue Jul 12, 2015

@ktoso

This comment has been minimized.

Copy link
Contributor

ktoso commented Jul 12, 2015

Addressed one part of the issue in this PR: #287
Will try to make time and figure out required_mustRequestFromUpstreamForElementsThatHaveBeenRequestedLongAgo soon.

Thanks again for reporting! Such reports are super useful in making the tests generally useful for all future implementers :-)

@RuedigerMoeller

This comment has been minimized.

Copy link
Author

RuedigerMoeller commented Jul 13, 2015

Hi, afair required_mustRequestFromUpstreamForElementsThatHaveBeenRequestedLongAgo had the same problem in the test prologue.
Will unignore the testcase and retry when out of office ;)

ktoso added a commit to ktoso/reactive-streams that referenced this issue Sep 15, 2015

ktoso added a commit to ktoso/reactive-streams that referenced this issue Sep 15, 2015

ktoso added a commit to ktoso/reactive-streams that referenced this issue Sep 16, 2015

@viktorklang

This comment has been minimized.

Copy link
Contributor

viktorklang commented Apr 6, 2016

@ktoso Was this fixed and merged?

@viktorklang

This comment has been minimized.

Copy link
Contributor

viktorklang commented Apr 6, 2016

@ktoso Sorry, I see the PR…

@ktoso

This comment has been minimized.

Copy link
Contributor

ktoso commented Apr 6, 2016

Yeap, it's that one which I need to get back to: #287

@viktorklang

This comment has been minimized.

Copy link
Contributor

viktorklang commented Dec 1, 2017

@RuedigerMoeller Please see #414. Will be released in 1.0.2 which is due before end-of-year!

@viktorklang viktorklang closed this Dec 1, 2017

@viktorklang viktorklang added this to the 1.0.2 milestone Dec 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.