Hi Pavel
I've been thinking afterwards, and I thought the best argument I can
possibly offer is that IMHO it is just wrong to try and project the
per-request resource requirements on the API itself. The simple matter
of fact is that I believe per-request resources are not suited to the
cases where something needs to be shared between the instances, be it
the AsyncResponse
(see the example here,
http://docs.oracle.com/javaee/7/api/javax/ws/rs/container/AsyncResponse.html),
or the broadcaster with SSE, etc.
The AsyncResponse example was obviously typed with per-request resources
in mind.
Some comments below (only to respond to your queries as opposed to
continuing arguing the case of not introducing the extra getBroadcaster
methods)
Thanks, Sergey
On 04/04/17 16:45, Pavel Bucek wrote:
>
>
> Hi Sergey,
>
> please see inline.
>
> On 31/03/2017 17:30, Sergey Beryozkin wrote:
>> 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 ?
>
> What would we do if the runtime can detect the scope? I really mean
> that as a question.
>
I thought it would let the would be new method implementations to
optimize the way the storage is done
> Sse instance can be share anywhere, it can be even accessed in
> "noscope" - custom thread, which runs "somewhere". Also, modifying
> method return value based on the scope seems to be rather confusing
> (same code used from different contexts would have different impact).
>
>> And if it has to be STATIC no matter what then I'm not sure it is
>> worth adding it to Sse.
>
> We've encountered an issue with injecting into constructors of
> singletons managed by other container than JAX-RS runtime. (i.e. CDI
> on some appserver). I'm not sure whether we can simply fix the issue
> or whether there is a lack of integration API for used runtime. I
> recall we tried to solve this some time ago - we hit the issue again
> when we are verifying the examples from the spec.
>
> Sergey, do you have any knowledge about that? Do you believe that
> constructor injection will always work / be usable?
>
> I'm asking intentionally in the context of this proposal, since if we
> allow "Sse" to have "static" context, we won't be forcing users
> working against possible integration pitfalls. I know that these
> issues are hard to deal with even when you have the control of the
> container code, which is usually not the case.
>
> I'm not saying that having static context is the only solution to this
> issue, but it would definitely help to have something like that.
I've found that if the constructor injection does not work then the
setter context injection always works, the end result is the same:
@Context
public void setSomeContext(...)
>
>>
>> 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.
>
> that's true; however I always saw the AsyncResponse usecase as:
>
> @Resource ManagedExecutorServicemanagedExecutorService;
>
> @GET public void get(@Suspended final AsyncResponse asyncResponse) {
>
> managedExecutorService.submit(() -> {
> try {
> // do something asyncResponse.resume("ok");
> }catch (Exception e) {
> asyncResponse.resume(e);
> }
> });
> }
> which indeed doesn't invalidate what you wrote.
I agree, the coordination between multiple threads entering diff
respurce methods is also a valid 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
> sure, we can certainly do that. Please if you have some time, let us
> know whether CXF does have any issues related to container-managed
> singleton constructor injection - I'm still not sure whether we can
> put a hard requirement like that into the specification, as I suspect
> that it might not be implementable without tight ties to the
> container. (thus it could make that usecase more environment specific,
> which is something we'd like to avoid if possible).
Singleton constructor injection should be covered 100% now in CXF if we
have Application.getClasses being initialized. Not covered 100% when the
classes are initialized from Spring. From CDI - not sure yet. However
I'm not sure what you mean about having the hard requirement, given that
I was suggesting that we just should follow the AsyncResponse example
with respect to supporting sharing of some 'contexts' for per-request
resources
Thanks, Sergey
> Thanks and regards, Pavel
>> 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
>>>>>>>>>>