users@jax-rs-spec.java.net

[jax-rs-spec users] [jsr339-experts] Re: Re: callbacks continued...

From: Marek Potociar <marek.potociar_at_oracle.com>
Date: Wed, 22 Aug 2012 22:32:39 +0200

FYI - the change is in: http://java.net/projects/jax-rs-spec/sources/git/revision/a31a86313a42ed5e72162676472839498096a3b6
I have decided to leave out onError() from CompletionCallback in the end as it didn't make sense when I was trying to document the method. Perhaps if someone could explain me better when the method would be called and why we need it, I can add it.

Marek

On Aug 21, 2012, at 4:38 PM, Marek Potociar <marek.potociar_at_oracle.com> wrote:

> To summarize my position in the discussion so far:
> Ad TimeoutHandler
> I want to avoid the need to define special behavior of register(...) method based on the type of registered callbacks as that would mean a significant expansion of the javadoc so that it's clear and understood by everyone. Thus I prefer to have only pure listener callbacks.
> Since the timeout handler is special as it is not just a pure listener, I'd like to keep it separate from the rest of the callbacks as well as enforce in the API that only one timeout handler is registered at a time.
> Ad ResumeCallback
> I'm fine with replacing this one with a Completion callback that would have onComplete(Response response) and onError(Throwable error) methods defined.
> I'm not convinced it makes sense to split this one into 2 separate interfaces. If a user is interested in completion events, he/she is most likely interested in any type of completion. Please correct me if I'm wrong.
> I'm not sure about the signature of these methods though (see point on sync callbacks).
> I'd prefer to also keep the original resume callback (invoked before response filtering / exception mapping / message sending), but I do not insist.
> Ad ConnectionCallback
> If you want, I can add it back in as an OPTIONAL callback API.
> Add supporting callbacks in synchronous scenarios
> It would be nice to agree on supporting callbacks as part of the synchronous scenarios as well. We should however keep that discussion separate from async case.
> I understand the limits of annotation-based approach, so, if anything, I'd say we should focus on interface-based solution.
> While we should keep the discussion separate from async, we should still make sure that the callback APIs are not tight to async scenarios (unless necessary).
>
> What's next in this area:
> We need to speed things up here. So, I'm going to do the following updates for now:
> Introduce CompletionCallback with the method signatures as above.
> Keep the ResumeCallback, but clarify the execution points.
> Re-introduce the ConnectionCallback and mark it as OPTIONAL.
> With the proposal updated as indicated above I want us to focus on discussing the specific details such as exact method signatures, what callbacks should be kept/ditched
> we can make slight changes even after PR is submitted, we just will need to follow more strict process of publishing any changes via JCP.
>
> Marek
>
> On Aug 17, 2012, at 4:23 PM, Bill Burke <bburke_at_redhat.com> wrote:
>
>>
>>
>> On 8/17/2012 5:55 AM, Marek Potociar wrote:
>>>
>>> On Aug 16, 2012, at 9:41 PM, Bill Burke <bburke_at_redhat.com
>>> <mailto:bburke_at_redhat.com>> wrote:
>>>
>>>>
>>>>
>>>> On 8/16/2012 2:35 PM, Marek Potociar wrote:
>>>>>
>>>>> On Aug 16, 2012, at 5:39 PM, Bill Burke <bburke_at_redhat.com
>>>>> <mailto:bburke_at_redhat.com>> wrote:
>>>>>
>>>>>> * Why have a separate setTimeoutHandler() method? Why not just
>>>>>> create and register a TimeoutCallback?
>>>>>
>>>>> because you may want to change the timeout values (e.g. from within
>>>>> the TimeoutHandler)
>>>>>
>>>>
>>>> Why can't this be implemented as a callback?
>>>>
>>>> TimeoutCallback.timeout(AsyncResponse res) {
>>>>
>>>> res.resume(Response.status(500).build());
>>>>
>>>> }
>>>
>>> For me, callbacks in this context are listeners. (We can rename to
>>> XxxListener if you want...) TimeoutHandler however is not a listener and
>>> thus since it may modify the processing state machine I want to have
>>> only one timeout handler registered at a time. I don't want to deal in
>>> spec with issues of one handler resuming a response and other suspending
>>> it etc. Thus I do not want to register timeout handler with other
>>> callbacks/listeners. Because it is a handler, not a listener.
>>>
>>
>> Since we already have a untyped register() method, whats the difference on whether its a handler or a callback or a listener? Each specific interface for each specific callback defines the semantics on what can and can't be done within it.
>>
>>
>>>>
>>>>>>
>>>>>> * register() returns a boolean in some instances and AsyncResponse
>>>>>> in others and the javadoc is off. IMO, you don't need a boolean
>>>>>> return value , instead throw an UnsupportedOperationException or
>>>>>> something.
>>>>>
>>>>> Fixed, nice catch. The decision to return boolean rather than failing
>>>>> is to provide optional (impl. specific) callback support. IOW, users
>>>>> may register even callbacks that are not understood and
>>>>> implementation should work normally, just without calling those
>>>>> callbacks. (See also the definition of OPTIONAL.)
>>>>>
>>>>>>
>>>>>> * If anything, ResumeCallback happens before response is sent, isn't
>>>>>> this what a ContainerResponseFilter already does? Also, how could
>>>>>> ResumeCallback modify the response?
>>>>>
>>>>> It doesn't. It's a listener.
>>>>>
>>>>
>>>> I don't get it then. Why invoke it before the actual response is sent
>>>> back if you can't modify the response?
>>>
>>> As I said later in my reply, I don't have a strong use case... off the
>>> top of my head - perhaps if you are doing some distributed computation
>>> or result search, you may use this event to terminate workers that are
>>> still computing? It's just another event - like "on error" or "on complete".
>>>
>>
>> I do have a use case for post-response sent. Again, the case where you want to commit a transaction if and only if the client successfully received the response, or rollback on an error.
>>
>>>> interface CompletionCallback {
>>>>
>>>> void completed(AsyncResponse res, Response res) {
>>>> }
>>>> }
>>>
>>> I wonder if this is something that could be useful also for sync
>>> invocations? But how to register it? Should we instead consider an
>>> annotation-based approach similar to CDI/EJB interceptors? E.g.
>>>
>>
>> You are right. Sync would be interested in Connection closed, error, and completion too. But, not sure I like an annotation approach. For something like @OnComplete in your example, how do you know what request was completed? @Callbacks requires callbacks with no state. The use cases I have for various callbacks will have to be initialized within code.
>>
>>> @Path("myresource")
>>> public class MyResource {
>>> @OnComplete
>>> public void completionCallback(CallbackEvent event) { ... }
>>>
>>> @Get
>>> @Callbacks(MyResource.class)
>>> public void doGet() { ... }
>>> }
>>>
>>> Obviously, that is something that would span beyond our async topic. And
>>> something that would have to be thought through more...
>>>
>>>> Called when there was an error sending the response back to the client.
>>>>
>>>> interface ErrorCallback {
>>>>
>>>> void error(AsyncResponse res, Response res) {
>>>> }
>>>> }
>>>
>>> Does the above make sense? What's in the Response? Where's the error
>>> information?
>>>
>>
>> Error happens on an I/O error back to the client. Since the socket layer could be throwing proprietary exceptions, what's the point of passing the exception along? I guess AsyncResponse and Response parameters aren't really needed.
>>
>>>>>
>>>>>> TimeoutCallback
>>>>>
>>>>> We have a TimeoutHandler. Since this is the special type of callback
>>>>> that may modify the execution state, I prefer we keep it separate
>>>>> from other callbacks that are intended to be used as pure listeners.
>>>>
>>>> If you look at the Servlet AsyncListener, onComplete() and onError()
>>>> are basically pure listeners, while onTimeout() allows you to modify
>>>> execution state.
>>>
>>> Yes, sadly, Servlet guys seem to have their own interpretation of term
>>> "listener". Also, they do not bother defining in their spec what happens
>>> if one such registered "listener" calls AsyncContext.complete() and then
>>> another one calls AsyncContext.dispatch(...). Leaves a space for
>>> implementation-specific differences that inhibit code portability.
>>>
>>> JAX-RS should not be a mere extension to a servlet spec IMO. It makes
>>> sense to cherry-pick the good parts of their API, but let's not try to
>>> mimic all their APIs including all their mistakes.
>>>
>>
>> Agreed, I was just pointing out that callbacks don't have to be limited to just listening.
>>
>>
>> --
>> Bill Burke
>> JBoss, a division of Red Hat
>> http://bill.burkecentral.com
>