Hi Pavel
May be it is just me, but it would seem quite strange, to have a method
getBroadcaster(String)
with the specific expectations of how it needs to be implemented. What
if a runtime can detect, in the context of the current request, that a
given resource is
singleton ? And if it has to be STATIC no matter what then I'm not sure
it is worth adding it to Sse.
Returning to the AsyncResponse, consider a per-request resource, one
method suspends the current invocation, and when some other method is
invoked, the task is resume the earlier suspended invocation. I can't
see how you can avoid having a static storage in the service class to
support this case.
I guess what I'm trying to say in the end is that I'd let per-request
resource users willing to deal with SSEBroadcasters simply handle them
locally in their code.
And only if we see JAX-RS 2.1 users working with per-request resources
actually complaining about it then a new Sse method can easily be added
in 2.2/etc without breaking the existing code
Sergey
On 31/03/17 13:44, Pavel Bucek wrote:
>
> Hi Sergey,
>
> sorry for not answering that one, I must have missed it :-)
>
> The intention is to have the storage STATIC, accessible from ANY
> scope. It must be thread safe.
>
> I'm also thinking about "The returned Broadcaster instance" must be
> open, which would imply "isOpen()" check and if that returns false,
> the instance would be replaced by a fresh one. (we don't have
> "removeBroadcaster(...)" yet and I hope we won't need to have that.
>
> Thanks and regards,
> Pavel
>
>
> On 31/03/2017 14:33, Sergey Beryozkin wrote:
>> 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
>>>>>>>>
>>>>
>>>
>>
>