Hi Sergey,
please see inline.
On 29/03/2017 16:34, Sergey Beryozkin wrote:
> Hi Pavel
>
> With the newBroadcaster() method I thought one would usually create it
> at the injection time and save it. If it is a singleton - Sse will
> allocate a single instance only. if it is per-request - Sse will
> allocate an instance of SseBroadcaster per request, from the service
> code's point of view it is still an instance of SseBroadcaster.
that might be our disconnect. Does it make sense to use "request-scoped"
broadcaster? That would imply that it will contain only single
SseEventSink and if not stored, it will be discarded after resource
method finishes. Which is not that useful - you could use SseEventSink
only and it would achieve the same functionality; opt1 is equivalent to opt2
// consider standard, request-scoped JAX-RS resource
@GET @Path("subscribe")
@Produces(MediaType.SERVER_SENT_EVENTS)
public void subscribe(@Context SseEventSink eventSink) {
// opt1 eventSink.onNext(sse.newEvent("msg"));
// opt2 SseBroadcaster broadcaster =sse.newBroadcaster();
broadcaster.subscribe(eventSink);
// broadcaster has only single subsrciber and will be discarded broadcaster.broadcast(sse.newEvent("msg"));
}
If we have additional methods like Sse#getBroadcaster(String ID) which
would get-or-create the broadcaster, it could be different:
@GET @Path("subscribe")
@Produces(MediaType.SERVER_SENT_EVENTS)
public void subscribe(@Context SseEventSink eventSink) {
// broadcaster has "x" subscribers SseBroadcaster broadcaster =sse.getBroadcaster(this.getClass());
// broadcaster has "x+1" subscribers, the instance is preserved broadcaster.subscribe(eventSink);
broadcaster.broadcast(sse.newEvent("msg"));
}
> I'm just curious, what side-effects will be waiting to happen if we
> introduce effectively a dedicate support for the static storage of the
> broadcasters.
I might not see something here, not sure whether there could be any
unwanted side-effect. The "Sse" instance would only become "real
context" instead of just a factory for creating broadcasters and
outbound events. Everything else can stay the same - in our prototype
implementation, Sse instance already is a singleton and there are
generally no issues with injecting singletons into any-scoped beans, as
long as the injected object is thread safe.
> I can see how it can help to support per-request service instances
> reuse the same SseBroadcaster instance, but what exactly does that
> achieve. save the runtime to create an instance of SseBroadcaster per
> request. A method like sse.getBroadcaster(Class) will not really make
> sense for the singletons, so I can see some ambiguity being introduced
> into Sse API ?
I don't see that as an ambiguity, maybe just as another usecase. JAX-RS
resources are by default request scoped, so it actually makes more sense
to have "get or create store", since SseBroadcaster instances are
expected to be retained for more than just a single request. The
"newBroadcaster" is still valid for some usecases, even when it could be
always replaced by "getOrCreate" with random id, but we are not removing
that now. Or at least I don't think about this like that. Thanks and
regards, Pavel
> Thanks, Sergey On 29/03/17 13:07, Pavel Bucek wrote:
>>
>> Ah, I did not mean it like anything is going away -
>> Sse#newBroadcaster will stay, only two method could be added to ease
>> "storing" and retrieving the broadcasters.
>>
>> It's meant as an addition, existing Sse methods will stay as they are
>> for now.
>>
>> Regards, Pavel
>>
>> On 29/03/2017 13:28, Sergey Beryozkin wrote:
>>> Hi Pavel In your original example you had a non static
>>> SseBroadcaster created from Sse.newBroadcaster. Yes, I recall it was
>>> also a singleton in your example, but as I said, I don't understand
>>> what difference does it make, @Context also works well for
>>> per-request resources. Is the actual problem is that SseBroadcasters
>>> are rather be 'static' in nature ? May be we can have
>>> sse.newBroadcaster() but add sse.getBroadcaster(Class) ? Cheers,
>>> Sergey On 29/03/17 12:14, Pavel Bucek wrote:
>>>>
>>>> Hi Sergey,
>>>>
>>>> that is still true, but with these new methods on Sse "context",
>>>> we'd remove the need for having JAX-RS Resources as singleton,
>>>> since the "store" on the Sse would be application-scoped.
>>>>
>>>> (let me add a code example to be extra sure we are on the same page)
>>>>
>>>> current state:
>>>>
>>>> (the synchronization could be done in a better way, please take it
>>>> only as an example)
>>>>
>>>> @Path("server-sent-events")
>>>> public class ServerSentEventsResource {
>>>>
>>>> private final Ssesse;
>>>>
>>>> private static SseBroadcasterbroadcaster;// ... per-class broadcaster storage public ServerSentEventsResource(@Context Sse sse) {
>>>> this.sse = sse;
>>>> }
>>>>
>>>> private static synchronized SseBroadcaster getBroadcaster(Sse sse) {
>>>> if (broadcaster ==null) {
>>>> broadcaster = sse.newBroadcaster();
>>>> }
>>>>
>>>> return broadcaster;
>>>> }
>>>>
>>>> @POST public void sendMessage(final String message)throws IOException {
>>>> getBroadcaster(sse).broadcast(sse.newEvent(message));
>>>> }
>>>>
>>>> // ... }
>>>>
>>>> future?:
>>>>
>>>> @Path("server-sent-events")
>>>> public class ServerSentEventsResource {
>>>>
>>>> private final Ssesse;
>>>>
>>>> public ServerSentEventsResource(@Context Sse sse) {
>>>> this.sse = sse;
>>>> }
>>>>
>>>> @POST public void sendMessage(final String message)throws IOException {
>>>> sse.getBroadcaster(this.getClass()).broadcast(sse.newEvent(message));
>>>> }
>>>>
>>>> // ... }
>>>>
>>>> Thanks, Pavel
>>>>
>>>> On 29/03/2017 12:34, Sergey Beryozkin wrote:
>>>>> HI Pavel That works for us. Recall that you said that 'Sse' object
>>>>> can be injected with a JAX-RS @Context and assuming the service
>>>>> resource is called DemoResource this works for either per-request
>>>>> or singleton, public class DemoResource { public
>>>>> DemoResource(@Context Sse sse) {} } For ex, if DemoService is
>>>>> per-request then the following works right now: public
>>>>> DemoResource(@Context Application app) {} Both 'contexts' are very
>>>>> much similar in that both are effectively the non-request specific
>>>>> factories Thanks, Sergey On 29/03/17 11:10, Pavel Bucek wrote:
>>>>>> Dear experts, we did a couple of changes in SSE API: - removed
>>>>>> SseEventSource.Builder#named(...) the intention was to name
>>>>>> threads used by the event source, but in the light of future
>>>>>> change, which should introduce "common" per-client thread pool(s)
>>>>>> for async operation, this won't be feasible to have there (since
>>>>>> it would require creating another threadpool per SseEventSource
>>>>>> instance) - SseEventSource now implements
>>>>>> Flow.Source<InboundSseEvent> seems like we'd need to have some
>>>>>> form of Publisher and Subscriber, currently named as Source and
>>>>>> Sink in the API. The main reason would be integration with other
>>>>>> frameworks - without it, it would be harder to do without a good
>>>>>> reason. Also, during the implementation and rewriting "old"
>>>>>> Jersey examples, one difficulty was discovered. It used to be
>>>>>> possible to create a broadcaster statically, which simulates "per
>>>>>> resource class" scope. The problem is that currently the only way
>>>>>> how to create a new SseBroadcaster is injecting Sse object. But
>>>>>> that is not available during static initialization and since
>>>>>> JAX-RS resources are by default request-scoped, storing the
>>>>>> SseBroadcaster instances might be a challenge for some users.
>>>>>> Does anyone see the same issue as we do? Would someone object if
>>>>>> we add methods like Sse#getBroadcaster(String)
>>>>>> Sse#getBroadcaster(Class<?>) which would serve as a storage for
>>>>>> broadcasters? (semantically it could be "get-or-create", passed
>>>>>> String or Class would serve as a key). Thanks and regards, Pavel
>>>