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

generic contextualizer methods to support reactive streams #180

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

njr-11
Copy link
Contributor

@njr-11 njr-11 commented Sep 24, 2019

pull fixes #166

This pull makes an attempt at addressing both of the scenarios for reactive streams from #166 in a generic manner that doesn't add reactive streams or Java 11 as compile dependencies to our spec classes.

Signed-off-by: Nathan Rauh [email protected]

@njr-11 njr-11 added the enhancement New feature or request label Sep 24, 2019
@njr-11 njr-11 requested a review from FroMage September 24, 2019 14:51
@njr-11 njr-11 self-assigned this Sep 24, 2019
@FroMage
Copy link
Contributor

FroMage commented Sep 25, 2019

Thanks.

What happens implementation-wise? I assume we can only proxy interface methods, so if we invoke onMethodsOf(Flowable) (Flowable is a class, subtype of RS Publisher) we can't really get back a Flowable so should it throw?

Why do we have two methods? I fail to see the difference between them.

@cescoffier any opinion?

@cescoffier
Copy link

It should return the same type as the passed instance, or you will get a naked Reactive Streams Publisher. If you do this, the user will recreate another object from this one, which is not very convenient.

Also, does it work with the subscribesOn and publishOn operator that switch the thread (if needed)?

@njr-11
Copy link
Contributor Author

njr-11 commented Sep 25, 2019

What happens implementation-wise? I assume we can only proxy interface methods, so if we invoke onMethodsOf(Flowable) (Flowable is a class, subtype of RS Publisher) we can't really get back a Flowable so should it throw?

It would proxy the interface that Flowable implements: Publisher. That means the subscribe method (the only one on the Publisher interface) would run with context.
This generic method won't be useful in every case. It was provided to cover the Subscriber pattern, not Publisher. What part of a Flowable instance were you hoping to apply context to? That class has an enormous number of methods.

Why do we have two methods? I fail to see the difference between them.

It is an attempt to address the concern you raised under #166 (comment) , which I've copied here:

For Subscriber it would be possible, because we capture on creation and restore on any method call, but if we wanted to also provide it for Publisher to make it automatic for every subscriber we could not because we don't want to capture and restore but just forward any calls to subscribe to our Subscriber wrapper.

So we have one method to fit the generic pattern of subscriber where you want all interface methods of the supplied instance to run with context, and a second method that fits the particular pattern that you outline for publisher, where instead subscribed instances are auto-contextualized such that their interface methods run with context. Personally, I didn't yet see a strong case for this second pattern because it seemed more of a minor convenience, but tried to add it to address the concern.

It should return the same type as the passed instance, or you will get a naked Reactive Streams Publisher. If you do this, the user will recreate another object from this one, which is not very convenient.

Also, does it work with the subscribesOn and publishOn operator that switch the thread (if needed)?

This seems to contradict what was asked for under #166. I'm fine if plans have changed, in which case we should cancel #166 and get a new issue opened to represent whatever the real requirements are. It sounds like you are wanting MicroProfile Context Propagation to take an existing instance from an external implementation and mutate it such that context is propagated within it. We can't go that far, but maybe we can define some sort of mechanism by which other implementations can better plug into MicroProfile Context Propagation to get context propagated when they need it. In the new issue, can you elaborate more fully on what you are trying to achieve?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ReactiveStreams
3 participants