RFR 8178699/10, Fail to send async requests if server doesn't response the first one

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8178699/10, Fail to send async requests if server doesn't response the first one

FELIX YANG

Hi all,

   please review the following patch to API in jdk.incubator.httpclient. The patch is more like a clean-up:

  • With current RequestProcessor implementation, constructing multiple HTTP Exchange may be blocked if the first request doesn't response by server side. This is introduced by synchronized setClient method together with other synchronized methods in BodyProcessor.
  • setClient/getClient in RequestProcessors.ProcessorBase and ResponseProcessors.AbstractProcessor are never used in real logic.
  • Such abstraction with implicit behavior looks to be confusing if someones wants to implement HttpResponse.BodyProcessor/HttpRequest.BodyProcessor themselves.

There are more detail root cause analysis in the bug.

Bug:

    https://bugs.openjdk.java.net/browse/JDK-8178699

Webrev:

    http://cr.openjdk.java.net/~xiaofeya/8178699/webrev.00/

Patch verified with jprt on all platforms and also attempted repeatedly locally.

Thanks,

Felix

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8178699/10, Fail to send async requests if server doesn't response the first one

Daniel Fuchs
Hi Felix,

Looks good to me.

+1 for removing unnecessary calls that need synchronization and
cause deadlocks.

If you still have the full thread-dump where the issue could
be observed it might be good to record it in JDK-8178699
(if you no longer have it then don't bother).

best regards,

-- daniel

On 23/06/2017 09:59, Felix Yang wrote:

> Hi all,
>
>     please review the following patch to API in
> jdk.incubator.httpclient. The patch is more like a clean-up:
>
>   * With current RequestProcessor implementation, constructing multiple
>     HTTP Exchange may be blocked if the first request doesn't response
>     by server side. This is introduced by synchronized setClient method
>     together with other synchronized methods in BodyProcessor.
>   * setClient/getClient in RequestProcessors.ProcessorBase and
>     ResponseProcessors.AbstractProcessor are never used in real logic.
>   * Such abstraction with implicit behavior looks to be confusing if
>     someones wants to implement
>     HttpResponse.BodyProcessor/HttpRequest.BodyProcessor themselves.
>
> There are more detail root cause analysis in the bug.
>
> Bug:
>
> https://bugs.openjdk.java.net/browse/JDK-8178699
>
> Webrev:
>
> http://cr.openjdk.java.net/~xiaofeya/8178699/webrev.00/
>
> Patch verified with jprt on all platforms and also attempted repeatedly
> locally.
>
> Thanks,
>
> Felix
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8178699/10, Fail to send async requests if server doesn't response the first one

FELIX YANG
Hi Daniel,
     thread dump can be found in JBS attachment .
Thanks
-Felix



Sent from my iphone

> 在 2017年6月23日,18:40,Daniel Fuchs <[hidden email]> 写道:
>
> Hi Felix,
>
> Looks good to me.
>
> +1 for removing unnecessary calls that need synchronization and
> cause deadlocks.
>
> If you still have the full thread-dump where the issue could
> be observed it might be good to record it in JDK-8178699
> (if you no longer have it then don't bother).
>
> best regards,
>
> -- daniel
>
>> On 23/06/2017 09:59, Felix Yang wrote:
>> Hi all,
>>    please review the following patch to API in jdk.incubator.httpclient. The patch is more like a clean-up:
>>  * With current RequestProcessor implementation, constructing multiple
>>    HTTP Exchange may be blocked if the first request doesn't response
>>    by server side. This is introduced by synchronized setClient method
>>    together with other synchronized methods in BodyProcessor.
>>  * setClient/getClient in RequestProcessors.ProcessorBase and
>>    ResponseProcessors.AbstractProcessor are never used in real logic.
>>  * Such abstraction with implicit behavior looks to be confusing if
>>    someones wants to implement
>>    HttpResponse.BodyProcessor/HttpRequest.BodyProcessor themselves.
>> There are more detail root cause analysis in the bug.
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8178699
>> Webrev:
>> http://cr.openjdk.java.net/~xiaofeya/8178699/webrev.00/
>> Patch verified with jprt on all platforms and also attempted repeatedly locally.
>> Thanks,
>> Felix
>