users@jax-rs-spec.java.net

[jax-rs-spec users] [jsr339-experts] Re: Re: Re: Re: Re: resume(Throwable) question

From: Marek Potociar <marek.potociar_at_oracle.com>
Date: Wed, 6 Feb 2013 16:09:16 +0100

On Feb 6, 2013, at 3:47 PM, Bill Burke <bburke_at_redhat.com> wrote:

>
>
> On 2/6/2013 9:31 AM, Sergey Beryozkin wrote:
>> On 06/02/13 14:16, Bill Burke wrote:
>>>
>>>
>>> On 2/6/2013 8:47 AM, Sergey Beryozkin wrote:
>>>> On 06/02/13 13:01, Bill Burke wrote:
>>>>>
>>>>>
>>>>> On 2/5/2013 6:37 PM, Marek Potociar wrote:
>>>>>>
>>>>>> On Feb 5, 2013, at 8:59 PM, Bill Burke <bburke_at_redhat.com> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2/5/2013 1:53 PM, Marek Potociar wrote:
>>>>>>>> I was under impression that leakage is implementation detail. We did
>>>>>>>> not have to specify anything for sync case, why should we specify it
>>>>>>>> for async case. There's nothing special about async case in this
>>>>>>>> regard.
>>>>>>>>
>>>>>>>
>>>>>>> In he sync case, the Servlet container is wrapping the JAX-RS
>>>>>>> invocation. So, exceptions can be propagated and handled by the
>>>>>>> servlet container if need be. In the async case, a non-servlet thread
>>>>>>> is handling the response. So, cleanup, exception handling, etc. needs
>>>>>>> to either be handled by the jax-rs layer, or user code. IMO, cleanup
>>>>>>> and exception handling should be done in the JAX-RS layer and should
>>>>>>> be as deterministic.
>>>>>>
>>>>>> Servlet is not the only platform JAX-RS runs on. Anyway, I think that
>>>>>> propagating the exception to the IO container is deterministic enough.
>>>>>> Whether or not this happens on the original thread is IMO orthogonal.
>>>>>> All IO containers that support async externalise all the necessary
>>>>>> connection-related context in one way or another and always provide a
>>>>>> facility for propagating exceptions back to this context. I have yet
>>>>>> to see a container that does not do that.
>>>>>>
>>>>>>>
>>>>>>>> That said, I'm fine with adding a short sentence to the
>>>>>>>> AR.resume()/cancel() javadoc stating that "A successful invocation
>>>>>>>> of this method eventually completes the suspended request
>>>>>>>> processing." or similar. FWIW, I do not want to write anything like
>>>>>>>> "when this method completes, the request processing is complete",
>>>>>>>> because I do not want to place restrictions on implementations. E.g.
>>>>>>>> implementors may decide to complete the response processing
>>>>>>>> asynchronously, which means that when the method returns the
>>>>>>>> connection does not have to be necessarily closed yet. Which is also
>>>>>>>> why I think it's better to not say anything...
>>>>>>>>
>>>>>>>
>>>>>>> Yeah, that's ok. If you want to force synchronicity, then you can
>>>>>>> just do it in the callback layer. It just wasn't very clear in the
>>>>>>> javadoc what was supposed to happen with unmapped exceptions. In the
>>>>>>> sync layer, the unmapped exception is rethrown. You just can't do
>>>>>>> that in the async layer, IMO.
>>>>>>
>>>>>> Yes, that's the main difference. While in sync case the propagation is
>>>>>> achieved by re-throwing and automatic rollback of the call stack, in
>>>>>> async case this propagation needs to happen in some other way. Yet
>>>>>> again, I have not seen any async-enabled IO container that would not
>>>>>> expose such facility.
>>>>>>
>>>>>
>>>>> Again, I think you are incorrect. If you do not have explicit language
>>>>> then the user does not know if they have to wrap resume() calls in
>>>>> try/catch blocks and call cancel. The current language of the javadoc
>>>>> makes it seem like unmapped exceptions are thrown from resume() and
>>>>> that
>>>>> the user needs to handle this. Which is why I started this thread in
>>>>> the
>>>>> first place...
>>>>>
>>>>> IMO, at least the javadoc should change from:
>>>>>
>>>>> "The processing of the data by JAX-RS framework follows the same
>>>>> path as
>>>>> * it would for the response data returned synchronously by a JAX-RS
>>>>> resource
>>>>> * method."
>>>>>
>>>>> To:
>>>>>
>>>>> "By executing this method, the request is guaranteed to be completed is
>>>>> some form or another. The processing of the data by the JAX-RS
>>>>> framework
>>>>> follows the same path as it would for the response data returned
>>>>> synchronously by a JAX-RS resource except that unmapped exceptions are
>>>>> not propagated. Depending on the JAX-RS implementation, unmapped
>>>>> exceptions will result in an error status being sent to the client
>>>>> and/or the connection being closed."
>>>>>
>>>>> I think the above gives jax-rs implementations enough flexibility, but
>>>>> makes it clear to the user how to interact with resume()/cancel().
>>>>>
>>>> While I can not say I always agree with Marek, but the current version
>>>> seems quite precise to me. Even in the sync case, the unmapped exception
>>>> will result in 500 being sent (by the container, on the servlet thread),
>>>> so why distinguish how this 500 may end up being reported by the
>>>> container in the async case ?
>>>>
>>>
>>> Maybe clear to you, but not clear to me. The resume() javadoc states
>>> that execution works the same as the sync case. If you read spec section
>>> 3.3.4 on Exceptions, bullet #4 states that unmapped and unhandled
>>> exceptions are wrapped and allowed to propagate to the container. There
>>> *IS* no container to propagate to in the async case. So, this leads me
>>> to believe that you have to have try/catch blocks around resume() calls.
>>>
>>> I think unmapped exceptions should be handled by resume() and cancel()
>>> by either sending an error code back to the client or closing the
>>> connection. This should be explicitly stated, or you'll have users like
>>> me misinterpreting the javadoc and having try/catch blocks around our
>>> resume() methods.
>>>
>> Hmm... So if the user resumes an invocation, by explicitly calling on
>> AsyncResponse:
>>
>> asyncResponse.resume()
>>
>> I'm presuming it is going to happen in some application thread,
>> so then the suspended thread will return to some function in the
>> application code, it throws the exception, right, so how come
>> one can expect to do
>>
>> try {
>> asyncResponse.resume(myObject)
>> } catch (Throwable t) {
>> }
>>
>> ?
>>
>> In other words,
>>
>> 1. Thread comes in and calls myJaxrsResourceMethod(@Suspended
>> AsyncResponse) and the application code just saves AsyncResponse in some
>> pool.
>>
>> 2. Next, the user code somehow resumes this AsyncResponse, all within
>> the application code, and the suspended thread returns, picks the
>> response Object up from AsyncResponse (with the help of the runtime),
>> without even entering the application code again, and proceeds same way
>> as it is done in the sync case.
>>
>> How do you expect the catch block in the above code be executed ?
>>
>> By the way, I'm not sure what do you mean there's no container in the
>> async case;
>
> I think Marek finally agreed with me, but I'll try to convince you too Sergey.

No, I did not. IMO there always is a container. What you perhaps wanted to point out that the resume may happen on a thread that is not managed by container and with a call-stack that does not originate in a container service call. Which does not mean that there is no container in async case though.

But I don't think it makes sense to fight about it and I acknowledge that I'm not objective here as I wrote the original javadoc. So, if there are users who may feel confused about it and there is a simple way how to lower the risk of confusion, it makes sense to apply the change IMO as the overall expected outcome is neutral to positive (users who are not confused will not get confused and users who are confused may get the clarification they need).

Marek

>
> IN the sync case, the servlet container wraps around the jaxrs invocation. So unmapped exceptions can be automatically propagated by just rethrowing the exception to the servlet container.
>
> In the async case, an application thread wraps around JAXRS response processing. If you follow section 3.3.4 religiously as the current resume() javadoc suggests, then unmapped exceptions are rethrown to this application thread. I don't think this is the right behavior.
>
>> I'll just add a test and see what happens when the
>> application code resumes AsyncResponse with Throwable which can not be
>> mapped and report back
>>
>
> Think about it from an implementation perspective. When you call resume() this results in:
>
> 1. executing filters
> 2. executing interceptors
> 3. executing the MBW
> 4. For SErvlet 3.0 implementations, calling asyncContext.complete() too.
>
> If an unmapped exception gets thrown from steps 1, 2, or 3 what do you do? The spec says the exception is propagated. resume() javadoc references the spec.
>
> --
> Bill Burke
> JBoss, a division of Red Hat
> http://bill.burkecentral.com