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