Hi Sergey,
thanks for digging in your email archive and bringing this up.
I just tried to write minimal SseBroadcaster usecase and this was the
result:
@Singleton @Path("/") public class SseResource { private final
SseContext sseContext; private final SseBroadcaster sseBroadcaster;
@Inject public SseResource(SseContext sseContext) { this.sseContext =
sseContext; this.sseBroadcaster = sseContext.newBroadcaster(); } @GET
@Path("eventStream") @Produces(MediaType.SERVER_SENT_EVENTS) public
SseEventOutput eventStream() { final SseEventOutput eventOutput =
sseContext.newOutput(); sseBroadcaster.subscribe(eventOutput); return
eventOutput; } public void onEvent(String event) {
sseBroadcaster.broadcast(sseContext.newDataEvent(event)); } }
I see what You and Andriy mean about broadcaster not being really
contextual, but .. I believe that's valid for all methods. Let's review
SseContext:
public interface SseContext { SseEventOutput
newOutput();OutboundSseEvent.Builder newEvent();SseBroadcaster
newBroadcaster(); // this is not pushed to the public repo yet :)
default OutboundSseEvent newDataEvent(String data) {}default
OutboundSseEvent newDataEvent(String data, String name) {}}
It's not really a context, it feels more like a factory. I don't see
anything related to current context - thread/request/application. I can
imagine an app (war) which would create new outbout event and pass it to
another war. And it would still work.
Also, the naming of the methods (new*) suggests that the "SseContext"
always produces new instances, it doesn't do a lookup in thread local or
any other store, it just creates instances.
Would rename to SseFactory (or any better name), help with the issue
raised by Andriy?
TBH, it doesn't feel right to limit the API in a way you've proposed - a
method with SseContext as a parameter would work / not work based on
from where it was called. I'm not saying there are no schemes like that
already, but here it feels more artificial and doesn't really help (the
error is still visible in the runtime, not before).
Also, I belive the sole purpose of having SseContext is to not have
builders for OutboundEvent, Broadcaster and EventOutput. We might even
consider removing SseContext and introduce those builders, but it would
make the connection from the API to the implementation slightly more
difficult. Or we can improve that, make it a "broadcaster store" -
something like "SseContext#getOrCreateBroadcaster(String Id)".. (that's
just me thinking out loud :).
What do you think?
Thanks and regards,
Pavel
On 08/02/2017 12:47, Sergey Beryozkin wrote:
> Hi Pavel
>
> Can you review the thread text below please (see a note from Marek).
> SSEContexts can be passed as method parameters, so when one creates a
> broadcaster it gives the impression the broadcaster is
> the current SSEContext specific
>
> Would it make sense to enforce that SSEBroadcaster can only be created
> if SSEContext is injected as a thread local proxy as opposed to a
> case where the broadcaster is created from SSEContext passed as a
> parameter (SSEContext.newBroadcaster() would throw the
> IllegalStateException is this case) ?.
>
>
> Thanks, Sergey
> On 15/08/16 12:58, Sergey Beryozkin wrote:
>> Hi Marek, Andriy, All
>>
>> FYI I tried to 'justify' the current design by the spec leads by the
>> requirement to have an implementation specific
>> SSEBroadcaster introduced without having to introduce an
>> SSEBroadcaster Context - the latter would make it possible to pass
>> SSEBroadcaster as a method parameter which might not be ideal.
>>
>> I agree with the Andriy's observation though that it does look like
>> it is a current SSEContext/event stream dependent if SSEContext is
>> passed
>> as a method parameter.
>>
>> Perhaps a neater way to work with broadcasters can be found :-)
>>
>> Sergey
>>
>>
>>
>> On 15/08/16 08:49, Marek Potociar wrote:
>>> Hi Andriy,
>>>
>>> This is a good observation - thank you for spotting this. I’ll give
>>> it some thought and come up with a fix proposal.
>>>
>>> Cheers,
>>> Marek
>>>
>>>> On 14 Aug 2016, at 04:36, Andriy Redko <drreta_at_gmail.com
>>>> <mailto:drreta_at_gmail.com>> wrote:
>>>>
>>>> Early feedback about JAX-RS 2.1 / SSE specification
>>>> Hey JAX-RS leads,
>>>>
>>>> As some of you may already know, the Apache CXF project is one of
>>>> the early adopters of
>>>> the JAX-RS 2.1 specification. In particular, we are currently
>>>> actively working on implementing
>>>> SSE support, based on drafted JAX-RS 2.1 API. Along the way we
>>>> stumbled upon some design
>>>> decisions which may require a bit of clarification or (hopefuly) a
>>>> second thought. The use case
>>>> in question is related to SseBroadcaster. Essentially, the idea
>>>> sounds pretty simple: events
>>>> into multiple SSE streams could be easily broadcasted. F.e. here is
>>>> a pseudo-code for typical
>>>> scenario:
>>>>
>>>> SseBroadcaster broadcaster = new SseBroadcasterImlp();
>>>>
>>>> broadcaster.register(stream1);
>>>> broadcaster.register(stream2);
>>>> ...
>>>>
>>>> broadcaster.broadcast(event1);
>>>> broadcaster.broadcast(event2);
>>>>
>>>> In nutshell, current Jersey's SSE implementation does exactly that.
>>>> However, in JAX-RS 2.1 API
>>>> draft SseBroadcaster could be created only from SseContext, f.e.:
>>>>
>>>> Response stream(@Context SseContext context) {
>>>> SseBroadcaster broadcaster = context.newBroadcaster();
>>>> broadcaster.register(context.newOutput());
>>>> ...
>>>> }
>>>>
>>>> It kind of works but totally defeats the idea of broadcaster as
>>>> being independent from SseContext or/and
>>>> streams it serves. Currently, every time new instance of the
>>>> SseBroadcaster will be created, with no
>>>> straightforward way to share it across multiple SSE streams. How it
>>>> could work though may look like that
>>>> (the implementation details could vary, just trying to present the
>>>> idea):
>>>>
>>>> private SseBroadcaster broadcaster = SseBroadcaster.newBroadcaster();
>>>>
>>>> Response stream1(@Context SseContext context) {
>>>> broadcaster.register(context.newOutput());
>>>> ...
>>>> }
>>>>
>>>> Response stream2(@Context SseContext context) {
>>>> broadcaster.register(context.newOutput());
>>>> ...
>>>> }
>>>>
>>>> In this case, the SseBroadcaster is truly context-independent, and
>>>> serves it purpose very well. We were trying to
>>>> come up with some realistic scenarios for SseBroadcaster utilizing
>>>> the current specification draft but did not really
>>>> succeeded. Surely, there are a few way to make it work but they do
>>>> not look good. Does it make sense? Whould you
>>>> mind guys to give some clarifications/guidance on that?
>>>>
>>>> Thank you a lot!
>>>>
>>>> Best Regards,
>>>> Andriy Redko