-
Notifications
You must be signed in to change notification settings - Fork 531
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
Add a Publisher
0..1 specialization
#490
base: master
Are you sure you want to change the base?
Conversation
* add a marker interface * add 5th paragraph to the specification
@viktorklang any chance to look at this PR? Cheers, |
Yes, I’ve seen it :)
Still on the fence on its inclusion though, needs to be argued further IMO.
👍
--
Cheers,
√
|
Although I agree that other types (single, completable, maybe) are required to express different high level APIs as many different libraries (reactor, rxjava, servicetalk, etc) have demonstrated, I do not think that the proposed change here actually addresses those concerns. Even if we provide a |
@NiteshKant thanks for the feedback!
Since
I see the point of "exactly one" and totally understand why some libraries have decided to have separate That said, I think I am not saying that we should not have |
I agree with @bsideup. Single, Maybe, Completable are derivable types from MonoPublisher. The goal is to define a generic type representing 0..1 regardless on whether it is guaranteed 1| guaranteed 0 | or maybe 0..1 | (if we talk more on such, we can have Error, which guaranteed to return an error and never onComplete or onNext) But my belief is that it is implementation, which less compatible when there is no 0..1 abstraction. Thus having MonoPublisher is a big step to say - hey, here we have a subtype of Publisher which guarantees to send no more than a single element |
❤️ , thanks for starting this discussion!
The value of introducing another type is that they can be used as arguments or return types to public APIs. In this context, the value of
Agreed, defining a contract for
So is |
I fully understand your point of trying to derive separate types, but consider the following.
Is it not that applicable to The following statements applicable for MonoPublisher:
That said when you have a It means that The same applies to the return type. If an API method returns Following the mentioned idea, having Clearly, we loose the semantic of stronger behavior of Sidenote: So, we left with the main question - do we need This question is addressed to anyone who participates in this thread and the answer should be derived from the following:
UpdateThinking about all of that, I tend to say If we thinking about r2db-spi which has
We expect:
So I have to say, that maybe we need Update 2On the other hand, we have RSocket project which has interactions such as Hence, I tend to say that we may need SinglePiblisher <| MonoPublisher <| Publishere which of course more challenging than MonoPublisher but brings more value than just a MonoPublisher which may still leave some questions. Cheers, |
Rite .. all connect APIs, HTTP response APIs, etc need For motivations for RSocket Drawing analogies for different types from the synchronous world, you have the following return type variants:
How often do you see similar methods while designing an API? There aren't may ways to tell which is more important than other, all are important I would say. |
Agree.
2xx is effective
In fact, we are good with
I see the problems... In the reactor, we usually solve the mentioned as the following
as
or
The second is indeed a bit ugly and would require
or
Agree, I can see how completable with generic can help...
agree
disagree, Java has this nullability problem so T can always be null -> effectively output is
now agree that But I need to say that all of these are done for the sake of better performance and in order to avoid Flux -> back to Mono conversion whilst the general semantic for the monoid stream does not exist in the spec at all which is more harmful (can be...) After all, in the expansion of the semantic, I can see the value of Well, folks invented Kotlin to always have That said |
I think the discussion got shifted in a wrong direction. Do we (as a reactive community) need Does it mean that we don't need much simpler 0..1 specialization of If we ever talk about adding Plus, if somebody questions the value of
I think "no different" is a stretch here. The bottom line is that we should not open the can of worms. Let's focus on |
Just to clarify.
Apologies for those long-reading, but I guess I tried to get @NiteshKant point.
As I stated, this is the main reason for this PR.
Agree with @bsideup and propose to raise the follow-up discussion on |
@bsideup I believe any of the 1 or less items types should not extend |
@NiteshKant I am afraid this will hurt the adoption. Also, they remain valid |
@NiteshKant what is your word on not extending I can see a performance reason, so having it as a separate type allows to get rid of redundant calls, but do you have anything in mind apart from performance optimizations? |
|
As much as I would like to reduce the overhead and simplify subscribing on such specialized I will be first to support the next initiative to apply the optimizations (RS spec 1.1?), but I have a feeling that it will take much longer, while WDYT? |
Here's a counter-proposal. Add method:
Notes:
Open issues:
|
Thanks for your proposal, Doug. I’d like to explore what is possible
without marker interfaces.
Here's a counter-proposal. Add method:
void Publisher.subscribe(Subscriber<? super T> subscriber, long
initialRequests, long maximumRequests);
I think the challenge would be that you’d have to know Subscriber
implementation details to figure out what to pass into subscribe(..), and
the Subscriber would no longer be in charge of its demand generation.
A counter-counter-proposal would be to add to Subscriber:
public long initialRequested() {
return 0;
}
Then all Publishers can query the Subscriber for initialRequested and start
onNext()-ing at-most that amount immediately after issuing onSubscribe.
It does require a bit of cleverness in the Subscription implementation, but
seems tractable.
Pairing initialRequested() with a method on Publisher:
public long maxElements() {
return -1;
}
a Processor could use both maxElements and initialRequested to optimize the
setup. (Negative is unknown, 0 is known-empty, Long.MAX_VALUE is unbounded).
Notes:
- This is in the spirit of "0-RTT" improvements in network protocols
(QUIC, TLS1.3, etc); the same rationales hold of removing one or more
request-response sequences.
- As usual requests of Long.MAX_VALUE mean effectively unbounded.
- The current one-arg version is equivalent to subscribe(s, 0,
Long.MAX_VALUE).
- It applies to the motivating use cases, and may also improve latency
in other common use cases
- It can be default-implemented without creating a subinterface.
- No type-introspection is needed (instead track maxRequests)
Open issues:
- Require (vs allow) auto-close when maxRequests hit?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#490 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACU5YFK6FAPZWBJSXN5MDRVJMRDANCNFSM4NMDYXYQ>
.
--
Cheers,
√
|
@DougLea are you suggesting that we add this method to
Although I like where the idea of adding Currently, there is no way to express 0..1 result with RS types. So I am kindly asking to keep in mind this important UX (not performance!) issue. |
@viktorklang I contemplated adding to subscription along these lines, but it seems better in all ways to overload a subscribe method: no RTTs if remote, and no thread-safety issues if local; plus simpler default implementability. Or maybe I'm missing something? |
@viktorklang @DougLea the purpose of that PR is to add an interface that identifies a Unfortunately, a Subscriber (even though such can specify that demand is exactly one element) has no relationship to the API, which users expose to expose a stream of elements. Optimizations that expose the subscriber demand in another way are out of spec, and far more than Reactive-Streams Spec should define (my point of view). |
I like the initial and max requested/elements proposals as it has both positives, 0-RTT and the ability for subscriber to assume there can be only one (or less) items. It also has the advantage that we do not have to add new interfaces. Perhaps |
I would agree with @viktorklang that these values detached from a I think having two additional methods on a
|
Additional constraints on the new methods would be that they will only be called before calling |
@bsideup If |
You're talking about the runtime. I am talking about the compile time / method signatures. Can we please stop ignoring the important aspect of being able to determine the 0..1 nature by looking at method's return type?Let's stop thinking about the optimizations for a second and think about the users who are struggling because they can't express / identify finite 0..1 methods just by using the RS.
|
Totally with @bsideup on this one, the fact there is no type in RS that allows detection of a single causes no end of pain for library authors and users. It causes problems in numerous areas such as runtime detection of the reactive type. In Micronaut for example we had to write a In addition as @bsideup said there is no way to write an API that isn't bound to a particular reactive library that expresses that the API emits a single value. To workaround this limitation we had to introduce an annotation in Micronaut called Which we are then forced to use to identify APIs that emit a single item. For example: All of these items make it harder than it should be to allow interoperability between Reactive libraries which is the fundamental goal that this PR is trying to resolve and surely a fundamental goal of Reactive Streams. I would absolutely love to see |
One more comment, this PR is also non-breaking as it introduces a new interface that can be gradually adopted by libraries. The discussion has veered towards introducing a new method which IMO is much more invasive and a greater breaker change for people that have adopted Reactive Streams. |
@bsideup and others: I'm addressing the protocol side of the lack of control for initially:n, max:m. Solving this makes it simpler to include corresponding specialized types if so desired; even ad-hoc non-standardized ones. While I'm at it: There's a difference between cancelling a subscription versus versus hitting max request bound versus cancelling a CompletableFuture: In the first case, you will eventually stop getting them. In the second, you won't get them at all past bound, and in the third case, you may get them but they will throw exceptions upon use. The second version probably most commonly applies but is not currently supported. A further aside, there was a vocal contingent (including some RS contributors) opposing including cancel in CompletionStage interface. One argument is along the lines of above, but I should have realized with RS that there should be some compensating RS features such as this. |
Hey @DougLea. Thank you for the clarification.
I belive such improvements are general enough and not fully related to this topic, thus I guess it is better to have a separate discussion for it. WDYT? Cheers, |
I've refrained as much as possible from commenting on the PR (and issue) so as not to pile up the discussion with Reactor's pov, but here are my most important thoughts on the points raised during these past few days:
|
My reservation with the SinglePublisher proposal is that response bounds are not necessarily properties of a Publisher or a Subscriber, but instead parameters of a protocol (i.e., Subscription), that cannot currently be expressed. Adding SinglePublisher without fixing this invites further flailing. |
From my POV, extending |
@simonbasle it looks like |
@NiteshKant |
In reactor null is used for 1 case - indicating an element when there is an ASYNC fusion mode, so this is a signal to drain the underlying queue, otherwise it is invalid Sent with GitHawk |
Also, the primary goal of |
@viktorklang ahha ... thanks! @simonbasle sorry about that point then 🙂 I guess
|
How does this change relate to the stated goal of Reactive Streams (from the README):
Note that there is nothing in the stated goal about providing an end-developer facing API, in fact, the second paragraph explicitly excludes such high level API concerns from the specification, and states that the spec is only about low level mediation, ie, runtime concerns. The goal of increased type safety seems to be one that I believe is focused on end-developer experience, not low level mediation, and so this use case, I believe, is not applicable to Reactive Streams. The JDK already has an asynchronous mono type - |
Indeed, this is a higher-level API, but the world needs it. It has shown the demand for many projects and all well know reactive libraries implemented such a type. That said, if not reactive-streams would be a holder of such API, then other initiatives will be created instead to provide a common standard Reactive Type for a lazy, cancellable stream of a single or empty value. |
@jroper thanks for joining the conversation!
While
|
Regarding this one: I would maybe agree if not the
the |
Agreed with @bsideup @OlegDokuka Even if it is stated in the spec that |
Even more so, what about
Are you saying that currently, RxJava, Reactor and Akka don't interop correctly between each other when using 0..1 elements? Because if that's the case, we need to put the brakes on right now and fix that, the APIs as they stand today are designed to be able to handle the 0..1 case, indeed there are TCK tests that ensure this.
I agree that
I agree that some optimisations can be done - though there are plenty of other optimisations too that could be done if we made even more richer types, for example, publishers that are backed by finite in memory collections have some pretty significant optimisations that can be made, failed publishers, subscribers that never back pressure, subscribers that just cancel, subscribers that only take a single element, etc. Where would we stop? And again this is where I come back to the purpose of reactive streams. The primary purpose is interop, performance is important but it is a secondary concern. Facilitating the primary purpose is ensuring that the API is as simple as possible. The fact that reactive streams is only 4 interfaces with 7 methods between them is key to achieving this goal (and as mentioned, one of these interfaces shouldn't even be there). Adding 20% more interfaces will complicate this, it will introduce more edge cases which need to be carefully discussed, and the semantics specified. The 0..1 publisher optimisations can be implemented without |
Sorry, but I don't think we're going to have a healthy and productive discussion if we mock each other and distort words. That's clearly not what @graemerocher said in his comment, and I bet you know that.
You can't do this optimization in RxJava without knowing about Reactor's type.
Yes, but without a common type, there isn't anything that helps the frameworks to "decide what can be optimised and how it will be optimised"
We stop at the very low hanging fruit, and it is the
I would agree here if we change the semantics for |
The TCK additions, examples and other changes can be added as a follow up (@OlegDokuka have volunteered to work on them if needed) to keep this PR small and focused.