On Aug 8, 2012, at 10:37 PM, Bill Burke <bburke_at_redhat.com> wrote:
> The AsyncResponse.register() methods should have their signature changed to:
>
> public AsyncResponse registerCallback(Class<?> callback, Class<?> callbacks) throws NullPointerException, IllegalArgumentException;
I assume you mean "Class<?>... callbacks". That would be ok provided we keep a single-argument version of the method. Otherwise one would get a "generic array" compiler warning every time a single callback class is registered.
> public AsyncResponse registerCallback(Object callback, Object... callbacks) throws NullPointerException, IllegalArgumentException;
This one might work perhaps even without a single-arg method version.
> Saves some typing when there is multiple callbacks. Also, IMO, renaming the method to registerCallback makes the method self describing. Otherwise you have to read the javadoc to know what it does.
Well, parameter name makes it IMHO self-describing enough. Also reading javadoc never hurts...
> Also, I do not think these methods belong on @Suspended
>
> Class<? extends TimeoutHandler> timeoutHandler() default DefaultTimeoutHandler.class;
> Class<?>[] callbacks() default {};
They have to be there so that a user has a chance of making sure the handler or callbacks get invoked in scenarios when the time-out interval is relatively small.
>
> DefaultTimeoutHandler should be removed as well.
Not possible unless we remove the stuff from annotation, which I'm against. That's the only reasonable way how to provide a default value for the timeoutHandler annotation property.
> IMO, having this kind of stuff in an annotation doesn't really provide any value or save any typing or generally make things easier on the developer. I would even make the case of removing timeout() or even the entire annotation. It's just a matter of what looks nicer?
I prefer we keep it in the annotation so that we can start experimenting with deployment descriptors that would be able to override the annotation-set defaults in our implementations. But if more people feel we should remove all attributes from @Suspended for now, I would reconsider this.
>
> public void suspended(@Suspended(timeout=1, timeUnit=SECONDS, callbacks=Foo.class, timeoutHandler=Timeout.class) AsyncResponse ) {...}
>
> vs.
>
> public void suspended(AsyncResponse response) {
>
> response.timeout(1, SECONDS).callbacks(Foo.class).timeoutHandler(Timeout.class);
>
> }
Yes, I agree - the second version is nicer. The only caveat is that it is not externally overridable. And alas, the timeout values are tightly bound to the timeout handler and callbacks, so having just timeout values in the @Suspended is simply not enough. The good thing is that you don't have to put those values in the annotation, if you don't need to.
Also, I do not like injection of the AsyncResponse without ANY qualifier annotation. Sure, for now it would work. But if you think about future and add a JAX-RS deployment descriptor into consideration, you'll see that not adding @Suspended now would force us to support 2 different async programming models in the future - one with @Suspended (or similar) annotation and one without, to preserve BW compatibility.
> I would say the second looks nicer. (if you add method chaining to AsyncResponse).
The method chaining is there already, isn't it.
> On 8/8/2012 3:06 PM, Bill Burke wrote:
>> I don't agree with @ManagedAsync. A method annotated with @ManagedAsync
>> is executed in a separate thread? The servlet container is *already*
>> doing this so I don't see the point of this annotation.
Apart from the fact that JAX-RS is not bound to run just on a servlet container, I believe you're also wrong when it comes to the assumption that a servlet is executed on a separate thread from the one that received the client connection. Servlet is (or at least can be) executed by exactly the same thread that received the client connection. (Because unless your servlet is async-dispatched, the client connection will close once the call to the servlet method terminates. IOW, async Servlet dispatching is not about async execution, it is about telling the Servlet container that it should not automatically close the client connection when the servlet call returns.) As you know the pool of these connection-handling threads is typically limited. And even if the servlet container had a special servlet execution thread pool, that pool would also be typically limited. Look at any async Servlet example - the request is either dispatched to a different servlet (with a possible special threadpool), which is however not that useful for JAX-RS resources, or a separate processing thread is manually started in the example.
In JAX-RS the suspended AsyncResponse tells the underlying container (such as Servlet) to not close the client connection (but suspend it instead) when the request processing returns on the primary execution thread. The @ManagedAsync addresses the same task as EJB @Asynchronous does - but for non-EJB resources: It guarantees that the resource method is called on a separate dedicated thread-pool that is managed by the JAX-RS runtime (the actual thread-pool configuration, management and provisioning is not defined by the spec).
>> Please make ConnectionCallback optional as I don't believe there is any
>> capability within Servlet 3 of providing that functionality.
Ok. Should we somehow indicate the capability?
>>
>> Everything else looks good.
Glad to hear. Also, I don't want my replies to sound dismissively, the things are open for a discussion, of course. It's just that in the last 2 weeks I evaluated 15 different variations of the async API in total and the proposed one seemed to provide the best pros:cons ratio, so some suggestions may not be new to me.
Marek
>>
>>
>> On 8/8/2012 2:36 PM, Marek Potociar wrote:
>>> Hello experts,
>>>
>>> Please review the latest async API changes:
>>> http://java.net/projects/jax-rs-spec/sources/git/revision/c13c30bf78e7a5113e6dcd7384ef663bb3db51eb
>>>
>>> I tried to make sure the javadoc is self-explanatory. You may also want
>>> to consult the async-related examples to see the API in action.
>>>
>>> Thanks,
>>> Marek
>>>
>>
>
> --
> Bill Burke
> JBoss, a division of Red Hat