eclipse / microprofile-context-propagation

Apache License 2.0
33 stars 23 forks source link

generic contextualizer methods to support reactive streams #180

Open njr-11 opened 4 years ago

njr-11 commented 4 years ago

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 nathan.rauh@us.ibm.com

FroMage commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 https://github.com/eclipse/microprofile-context-propagation/issues/166#issuecomment-506729664 , 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?