Hi Sergey,
please see inline.
On 30/03/2017 13:18, Sergey Beryozkin wrote:
> Hi Pavel
>
> Thanks, sorry it was not a good argument on my side, you are right,
> per-request subscriber instances do not make sense.
> I suppose what I'm trying to say is that we will have 2 methods for
> creating the subscribers, one which makes sense for per-request resources
> (Sse#getBroadcaster(Class)) and one for the singletons
> (Sse#newBroadcaster())
it's a combination, user of that API just has to know what he does.
List of valid usecases:
per request:
- newBroadcaster() with custom broadcaster instances storage
- getBroadcaster(ID) with and ID per resource / message domain
singleton
- newBroadcaster() in constructor, when storing "per-class" or "per
message domain" broadcaster to a final field
- getBroadcaster(ID) when we don't want to store anything and
delegate it to the JAX-RS runtime
I believe I could put together couple more..
> As a side note, note sure about the RI community but I think (possibly
> mistakenly) that singletons are primarily used by CXF users;
> irrespectively of that though, Application.getSingletons is here to
> support them.
I'm not exactly sure about the usecase, but Application.getSingletons
returns Set<Object>, which is very inconvenient to use by any means
other than providing objects to the application bootstrap. (i.e.: I
don't see it being used during the runtime to retrieve some concrete
singleton). Not to mention that only @Path and @Provider annotated
classes should be there.
(Not really sure whether there is a requirement for
javax.ws.rs.core.Feature implementations to have @Provider (I do see
that Jersey doesn't really honor that), but the javadoc of the
Application#getSingletons is quite clear about what should be there).
>
> Now, we will have
>
> Sse#newBroadcaster()
> Sse#getBroadcaster(Class)
please consider Class as an ID. I believe I indicated that we consider 2
methods for addition:
Sse#getBroadcaster(String)
Sse#getBroadcaster(Class).
The latter could be something like:
default SseBroadcaster getBroadcaster(Class<?> clazz) {
return getBroadcaster(clazz.getName());
}
the overload with class is just a helper method to ease doing the "class
scoped" instance, which seems to be the most common case.
>
> these methods are siblings at the API level hence either of them
> should make sense for the service code working with Sse but we know
> that one method will actually make sense if the service is running as
> a singleton, another one - as a per-request.
as I already mentioned, both can be used, no matter whether in Singleton
or Request scope.
>
> The other thing (I thought about it before you started this thread)
> that the broadcasters can be grouped by URIs, ex, given 4 services
> methods which can support the SSE connections, 2 of those methods will
> have its own broadcaster, and 2 other methods - its own broadcaster.
> Sse#getBroadcaster(Class) will not help.
sure, we can have Sse#getBroadcaster(String).
> Likewise, if a singleton calls Sse#getBroadcaster(Class), should Sse
> simply return a new instance or keep it in the static storage too ?
I wouldn't define the behavior based on the context; Sse instance would
be a singleton and the storage would be a singleton map as well, so all
methods would behave the same way. Remember, Sse instance can be stored
and accessed outside of any scope (for example in external event handler).
> Having typed all of it, I'm thinking now, may be it is OK to have 2 of
> these methods - the side-effects will be documented, ex, that
> getBroadcaster(Class) will create a static instance, though it is a
> bit unusual ?
I was hesitating a little when the proposal came (as I mentioned, it was
based on an experience when rewriting existing Jersey SSE examples), but
it makes more sense when you compare the code with and without these
methods.
> Do you think that per-request apps can simply have a static
> ConcurrentHashMap<Class, SseBroadcaster> in their code if needed and
> use Sse#newBroadcaster() would be unreasonable ? Ex, check
> AsyncResponse docs, API does not offer any support for keeping
> AsyncResponse instances, and I guess if it is per-request then the
> same issue applies
I see your point and it is an interesting analogy, but I believe that
the usecase of AsyncResponse is very different - it doesn't make sense
to store AsyncResponse instance as a field in a singleton. I can be
mistaken of course :)
Thanks and regards,
Pavel
>
> Thanks Sergey
>
>
> On 30/03/17 08:51, Pavel Bucek wrote:
>>
>> 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
>>>>>
>