On Aug 16, 2012, at 5:39 PM, Bill Burke <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)
>
> * 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.
> Call AsyncResponse.resume()?
Should not, but implementations should prevent this.
> If so, how can you avoid an infinite loop?
Up to implementation to prevent this.
> I question all this behavior.
>
> * I don't like the current behavior of ResumeCallback and think it should be removed.
>
> * I think we should have similar Servlet 3.0 callbacks.
>
> CompletionCallback
> OnErrorCallback
Please define the API for those 2.
> 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.
>
> OnComplete and OnError callbacks are very interesting. Consider the case where you only want to commit a transaction if the response was sent to the client and want to rollback if it wasn't. ResumeCallback doesn't handle either of those.
I do not have a compelling use case for resume callback. It just seemed to me that it may be useful to learn about the request processing getting resumed. So I'm fine with either of these:
- redefine the resume callback to provide the functionality above, or
- split that API into these two, or
- keep the ResumeCallback as is and add these two.
Marek
>