RFR (9): 8180498 Remove httpclient internal APIs which supply ByteBuffers to read calls

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

RFR (9): 8180498 Remove httpclient internal APIs which supply ByteBuffers to read calls

Michael McMahon
Could I get the following change reviewed please?

Bug: https://bugs.openjdk.java.net/browse/JDK-8180498

Webrev: http://cr.openjdk.java.net/~michaelm/8180498/webrev.1/

Thanks,
Michael.
Reply | Threaded
Open this post in threaded view
|

Re: RFR (9): 8180498 Remove httpclient internal APIs which supply ByteBuffers to read calls

Daniel Fuchs
Hi Michael,

Looks good - but what purpose does the rxBuffer now
serves in Exchange.java?

If I'm not mistaken it's an empty 0 sized buffer that
is never replaced... At the very least it should be
final (and not volatile), but maybe getBuffer() could
simply be removed here?

best regards,

-- daniel

On 19/05/2017 15:29, Michael McMahon wrote:
> Could I get the following change reviewed please?
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8180498
>
> Webrev: http://cr.openjdk.java.net/~michaelm/8180498/webrev.1/
>
> Thanks,
> Michael.

Reply | Threaded
Open this post in threaded view
|

Re: RFR (9): 8180498 Remove httpclient internal APIs which supply ByteBuffers to read calls

Michael McMahon
You're right Daniel. It's actually a vestige of the original intention
to support pipelined requests, which are not supported. getBuffer() is
actually
called from Http1Response, but it would be better to get the empty
buffer reference
at that point.

Thanks
Michael

On 19/05/2017, 17:42, Daniel Fuchs wrote:

> Hi Michael,
>
> Looks good - but what purpose does the rxBuffer now
> serves in Exchange.java?
>
> If I'm not mistaken it's an empty 0 sized buffer that
> is never replaced... At the very least it should be
> final (and not volatile), but maybe getBuffer() could
> simply be removed here?
>
> best regards,
>
> -- daniel
>
> On 19/05/2017 15:29, Michael McMahon wrote:
>> Could I get the following change reviewed please?
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8180498
>>
>> Webrev: http://cr.openjdk.java.net/~michaelm/8180498/webrev.1/
>>
>> Thanks,
>> Michael.
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (9): 8180498 Remove httpclient internal APIs which supply ByteBuffers to read calls

Michael McMahon
Daniel

I've updated the webrev at
http://cr.openjdk.java.net/~michaelm/8180498/webrev.2/

The diffs between the two versions is the following:

Thanks,
Michael.

./src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Exchange.java

36d35
< import java.nio.ByteBuffer;
79,81d77
<     // buffer for receiving response headers
<     private volatile ByteBuffer rxBuffer = Utils.EMPTY_BYTEBUFFER;
<
124,127d119
<     ByteBuffer getBuffer() {
<         return rxBuffer;
<     }
<

./src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http1Exchange.java

77c77
<         this.buffer = exchange.getBuffer();
---
 >         this.buffer = Utils.EMPTY_BYTEBUFFER;

---
 >         this.buffer = Utils.EMPTY_BYTEBUFFER;


On 19/05/2017, 18:01, Michael McMahon wrote:

> You're right Daniel. It's actually a vestige of the original intention
> to support pipelined requests, which are not supported. getBuffer() is
> actually
> called from Http1Response, but it would be better to get the empty
> buffer reference
> at that point.
>
> Thanks
> Michael
>
> On 19/05/2017, 17:42, Daniel Fuchs wrote:
>> Hi Michael,
>>
>> Looks good - but what purpose does the rxBuffer now
>> serves in Exchange.java?
>>
>> If I'm not mistaken it's an empty 0 sized buffer that
>> is never replaced... At the very least it should be
>> final (and not volatile), but maybe getBuffer() could
>> simply be removed here?
>>
>> best regards,
>>
>> -- daniel
>>
>> On 19/05/2017 15:29, Michael McMahon wrote:
>>> Could I get the following change reviewed please?
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8180498
>>>
>>> Webrev: http://cr.openjdk.java.net/~michaelm/8180498/webrev.1/
>>>
>>> Thanks,
>>> Michael.
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (9): 8180498 Remove httpclient internal APIs which supply ByteBuffers to read calls

Daniel Fuchs
On 22/05/2017 15:38, Michael McMahon wrote:
> Daniel
>
> I've updated the webrev at
> http://cr.openjdk.java.net/~michaelm/8180498/webrev.2/
>
> The diffs between the two versions is the following:

Looks good Michael!

-- daniel

>
> Thanks,
> Michael.
>
> ./src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Exchange.java
>
>
> 36d35
> < import java.nio.ByteBuffer;
> 79,81d77
> <     // buffer for receiving response headers
> <     private volatile ByteBuffer rxBuffer = Utils.EMPTY_BYTEBUFFER;
> <
> 124,127d119
> <     ByteBuffer getBuffer() {
> <         return rxBuffer;
> <     }
> <
>
> ./src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http1Exchange.java
>
>
> 77c77
> <         this.buffer = exchange.getBuffer();
> ---
>>         this.buffer = Utils.EMPTY_BYTEBUFFER;
>
> ---
>>         this.buffer = Utils.EMPTY_BYTEBUFFER;
>
>
> On 19/05/2017, 18:01, Michael McMahon wrote:
>> You're right Daniel. It's actually a vestige of the original intention
>> to support pipelined requests, which are not supported. getBuffer() is
>> actually
>> called from Http1Response, but it would be better to get the empty
>> buffer reference
>> at that point.
>>
>> Thanks
>> Michael
>>
>> On 19/05/2017, 17:42, Daniel Fuchs wrote:
>>> Hi Michael,
>>>
>>> Looks good - but what purpose does the rxBuffer now
>>> serves in Exchange.java?
>>>
>>> If I'm not mistaken it's an empty 0 sized buffer that
>>> is never replaced... At the very least it should be
>>> final (and not volatile), but maybe getBuffer() could
>>> simply be removed here?
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> On 19/05/2017 15:29, Michael McMahon wrote:
>>>> Could I get the following change reviewed please?
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8180498
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~michaelm/8180498/webrev.1/
>>>>
>>>> Thanks,
>>>> Michael.
>>>