Hi Pavel
Thanks, it all sounds reasonable, though the question will remain, given
getBroadcaster(String)
how to implement this method - the implementation will not know if it is
a singleton or per-request resource which is calling it, hence
how would it know if should do a static storage or not
Re Application.getSingletons - I was only referring to it to imply that
singleton resources can be easily preferred if needed (it was probably
off-topic)
Thanks, Sergey
On 30/03/17 18:21, Pavel Bucek wrote:
>
> 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
>>>>>>
>>
>