RFR 8191494: Refresh the incubating Http Client

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

RFR 8191494: Refresh the incubating Http Client

Chris Hegarty
Work on the incubating HTTP Client has been ongoing in the
`http-client-branch` of the JDK sandbox [1] ( and previously in the JDK
10 sandbox [2] ). This issue proposes to take a snapshot of that work so
as to refresh the version in the JDK mainline. As of now the mainline
that corresponds to JDK 10.

While incubating in JDK 9, the implementation has been almost completely
rewritten over in the sandbox. The implementation is now complete
asynchronous ( previously HTTP/1.1 has a blocking implementation ). Use
of the RX Flow concept been pushed down into the implementation. This
eliminates much of the original custom concepts to support HTTP/2. The
"flow" of data can now be more easily traced from the user-level request
publishers and response subscribers, all the way down to the underlying
socket. This significantly reduces the number of concepts and complexity
in the code, and maximizes the possibility of reuse between HTTP/1.1 and
HTTP/2.

As the API is still incubating, there have been some API tweaks. Mainly
renaming ( request & response body processors are now request publisher
and response subscriber ), minor spec clarifications around exceptions,
a general tidy up and changes to address a number of external feedback
items. Additionally, much work has been done on increasing test coverage
and stabilization of the tests themselves.

http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/

The webrev contains contributions from Chris Hegarty, Daniel Fuchs
Michael McMahon, and Pavel Rappo. As can be seen from the sandbox
history, review of the code has been effectively ongoing as the code has
evolved, but nonetheless I'm sure it will benefit from further review.

-Chris.

[1] hg clone http://hg.openjdk.java.net/jdk/sandbox; cd sandbox; hg
update http-client-branch
[2] http://hg.openjdk.java.net/jdk10/sandbox/
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8191494: Refresh the incubating Http Client

Chris Hegarty
Just an update on this.

There have been many review comments, off line, that have resulted in
changes pushed to the sandbox, so I've refreshed the webrev at the
same location.

http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/

-Chris.

On 17/11/17 18:31, Chris Hegarty wrote:

> Work on the incubating HTTP Client has been ongoing in the
> `http-client-branch` of the JDK sandbox [1] ( and previously in the JDK
> 10 sandbox [2] ). This issue proposes to take a snapshot of that work so
> as to refresh the version in the JDK mainline. As of now the mainline
> that corresponds to JDK 10.
>
> While incubating in JDK 9, the implementation has been almost completely
> rewritten over in the sandbox. The implementation is now complete
> asynchronous ( previously HTTP/1.1 has a blocking implementation ). Use
> of the RX Flow concept been pushed down into the implementation. This
> eliminates much of the original custom concepts to support HTTP/2. The
> "flow" of data can now be more easily traced from the user-level request
> publishers and response subscribers, all the way down to the underlying
> socket. This significantly reduces the number of concepts and complexity
> in the code, and maximizes the possibility of reuse between HTTP/1.1 and
> HTTP/2.
>
> As the API is still incubating, there have been some API tweaks. Mainly
> renaming ( request & response body processors are now request publisher
> and response subscriber ), minor spec clarifications around exceptions,
> a general tidy up and changes to address a number of external feedback
> items. Additionally, much work has been done on increasing test coverage
> and stabilization of the tests themselves.
>
> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>
> The webrev contains contributions from Chris Hegarty, Daniel Fuchs
> Michael McMahon, and Pavel Rappo. As can be seen from the sandbox
> history, review of the code has been effectively ongoing as the code has
> evolved, but nonetheless I'm sure it will benefit from further review.
>
> -Chris.
>
> [1] hg clone http://hg.openjdk.java.net/jdk/sandbox; cd sandbox; hg
> update http-client-branch
> [2] http://hg.openjdk.java.net/jdk10/sandbox/
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8191494: Refresh the incubating Http Client

Daniel Fuchs
Hi Chris,

Thanks for refreshing the webrev with our internal feedback.

I believe there are still some cleanup we could do to
wean out some more dead code (for instance I believe
ExceptionallyCloseable though implemented - is not
really used anywhere).

There are also some files that contain some commented out
code that could be cleaned up (e.g. HeaderParser.java,
Http1Request.java, Http2Connection.java, SSLDelegate.java)

Http2Connection.java:
Obsolete comments can be removed:
  212     // only keep a strong reference to Http2ClientImpl, which only has
  213     // a weak reference on HttpClientImpl, to avoid strong references
  214     // from the selector thread to HttpClientImpl (via attachments).


These are mostly nit but I think we should at least delete
the obsolete commented out method from Http2Connection as
there are too many of them and some of these are confusing.

Also your webrev is missing a recent change I pushed to add
copyright headers to policy files, as well as some minor
other fixes, hopefully you'll be able to take that in before
integrating.

best regards,

-- daniel

On 24/11/2017 17:05, Chris Hegarty wrote:

> Just an update on this.
>
> There have been many review comments, off line, that have resulted in
> changes pushed to the sandbox, so I've refreshed the webrev at the
> same location.
>
> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>
> -Chris.
>
> On 17/11/17 18:31, Chris Hegarty wrote:
>> Work on the incubating HTTP Client has been ongoing in the
>> `http-client-branch` of the JDK sandbox [1] ( and previously in the
>> JDK 10 sandbox [2] ). This issue proposes to take a snapshot of that
>> work so as to refresh the version in the JDK mainline. As of now the
>> mainline that corresponds to JDK 10.
>>
>> While incubating in JDK 9, the implementation has been almost
>> completely rewritten over in the sandbox. The implementation is now
>> complete asynchronous ( previously HTTP/1.1 has a blocking
>> implementation ). Use of the RX Flow concept been pushed down into the
>> implementation. This eliminates much of the original custom concepts
>> to support HTTP/2. The "flow" of data can now be more easily traced
>> from the user-level request publishers and response subscribers, all
>> the way down to the underlying socket. This significantly reduces the
>> number of concepts and complexity in the code, and maximizes the
>> possibility of reuse between HTTP/1.1 and HTTP/2.
>>
>> As the API is still incubating, there have been some API tweaks.
>> Mainly renaming ( request & response body processors are now request
>> publisher and response subscriber ), minor spec clarifications around
>> exceptions, a general tidy up and changes to address a number of
>> external feedback items. Additionally, much work has been done on
>> increasing test coverage and stabilization of the tests themselves.
>>
>> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>>
>> The webrev contains contributions from Chris Hegarty, Daniel Fuchs
>> Michael McMahon, and Pavel Rappo. As can be seen from the sandbox
>> history, review of the code has been effectively ongoing as the code
>> has evolved, but nonetheless I'm sure it will benefit from further
>> review.
>>
>> -Chris.
>>
>> [1] hg clone http://hg.openjdk.java.net/jdk/sandbox; cd sandbox; hg
>> update http-client-branch
>> [2] http://hg.openjdk.java.net/jdk10/sandbox/

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8191494: Refresh the incubating Http Client

Chris Hegarty
Thanks for the review Daniel.

> On 28 Nov 2017, at 18:22, Daniel Fuchs <[hidden email]> wrote:
>
> Hi Chris,
>
> Thanks for refreshing the webrev with our internal feedback.

You’re welcome.

> I believe there are still some cleanup we could do to
> wean out some more dead code (for instance I believe
> ExceptionallyCloseable though implemented - is not
> really used anywhere).

I see that you have already resolved this. Thanks

> There are also some files that contain some commented out
> code that could be cleaned up (e.g. HeaderParser.java,
> Http1Request.java, Http2Connection.java, SSLDelegate.java)
>
> Http2Connection.java:
> Obsolete comments can be removed:
> 212     // only keep a strong reference to Http2ClientImpl, which only has
> 213     // a weak reference on HttpClientImpl, to avoid strong references
> 214     // from the selector thread to HttpClientImpl (via attachments).
>
>
> These are mostly nit but I think we should at least delete
> the obsolete commented out method from Http2Connection as
> there are too many of them and some of these are confusing.

Done.

> Also your webrev is missing a recent change I pushed to add
> copyright headers to policy files, as well as some minor
> other fixes, hopefully you'll be able to take that in before
> integrating.

I refreshed the webrev, and it now contains these changes.

http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/

---

If it’s not already obvious, I have also done a review of this
code, and pushed a number of changes to the sandbox
branch to resolve issues that arose during review.

-Chris.

> best regards,
>
> -- daniel
>
> On 24/11/2017 17:05, Chris Hegarty wrote:
>> Just an update on this.
>> There have been many review comments, off line, that have resulted in
>> changes pushed to the sandbox, so I've refreshed the webrev at the
>> same location.
>> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>> -Chris.
>> On 17/11/17 18:31, Chris Hegarty wrote:
>>> Work on the incubating HTTP Client has been ongoing in the `http-client-branch` of the JDK sandbox [1] ( and previously in the JDK 10 sandbox [2] ). This issue proposes to take a snapshot of that work so as to refresh the version in the JDK mainline. As of now the mainline that corresponds to JDK 10.
>>>
>>> While incubating in JDK 9, the implementation has been almost completely rewritten over in the sandbox. The implementation is now complete asynchronous ( previously HTTP/1.1 has a blocking implementation ). Use of the RX Flow concept been pushed down into the implementation. This eliminates much of the original custom concepts to support HTTP/2. The "flow" of data can now be more easily traced from the user-level request publishers and response subscribers, all the way down to the underlying socket. This significantly reduces the number of concepts and complexity in the code, and maximizes the possibility of reuse between HTTP/1.1 and HTTP/2.
>>>
>>> As the API is still incubating, there have been some API tweaks. Mainly renaming ( request & response body processors are now request publisher and response subscriber ), minor spec clarifications around exceptions, a general tidy up and changes to address a number of external feedback items. Additionally, much work has been done on increasing test coverage and stabilization of the tests themselves.
>>>
>>> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>>>
>>> The webrev contains contributions from Chris Hegarty, Daniel Fuchs Michael McMahon, and Pavel Rappo. As can be seen from the sandbox history, review of the code has been effectively ongoing as the code has evolved, but nonetheless I'm sure it will benefit from further review.
>>>
>>> -Chris.
>>>
>>> [1] hg clone http://hg.openjdk.java.net/jdk/sandbox; cd sandbox; hg update http-client-branch
>>> [2] http://hg.openjdk.java.net/jdk10/sandbox/
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8191494: Refresh the incubating Http Client

Daniel Fuchs
Hi Chris,

Thanks for taking on the changes.
The webrev looks good.

best regards,

-- daniel

On 30/11/2017 13:00, Chris Hegarty wrote:

> Thanks for the review Daniel.
>
>> On 28 Nov 2017, at 18:22, Daniel Fuchs <[hidden email]> wrote:
>>
>> Hi Chris,
>>
>> Thanks for refreshing the webrev with our internal feedback.
>
> You’re welcome.
>
>> I believe there are still some cleanup we could do to
>> wean out some more dead code (for instance I believe
>> ExceptionallyCloseable though implemented - is not
>> really used anywhere).
>
> I see that you have already resolved this. Thanks
>
>> There are also some files that contain some commented out
>> code that could be cleaned up (e.g. HeaderParser.java,
>> Http1Request.java, Http2Connection.java, SSLDelegate.java)
>>
>> Http2Connection.java:
>> Obsolete comments can be removed:
>> 212     // only keep a strong reference to Http2ClientImpl, which only has
>> 213     // a weak reference on HttpClientImpl, to avoid strong references
>> 214     // from the selector thread to HttpClientImpl (via attachments).
>>
>>
>> These are mostly nit but I think we should at least delete
>> the obsolete commented out method from Http2Connection as
>> there are too many of them and some of these are confusing.
>
> Done.
>
>> Also your webrev is missing a recent change I pushed to add
>> copyright headers to policy files, as well as some minor
>> other fixes, hopefully you'll be able to take that in before
>> integrating.
>
> I refreshed the webrev, and it now contains these changes.
>
> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>
> ---
>
> If it’s not already obvious, I have also done a review of this
> code, and pushed a number of changes to the sandbox
> branch to resolve issues that arose during review.
>
> -Chris.
>
>> best regards,
>>
>> -- daniel
>>
>> On 24/11/2017 17:05, Chris Hegarty wrote:
>>> Just an update on this.
>>> There have been many review comments, off line, that have resulted in
>>> changes pushed to the sandbox, so I've refreshed the webrev at the
>>> same location.
>>> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>>> -Chris.
>>> On 17/11/17 18:31, Chris Hegarty wrote:
>>>> Work on the incubating HTTP Client has been ongoing in the `http-client-branch` of the JDK sandbox [1] ( and previously in the JDK 10 sandbox [2] ). This issue proposes to take a snapshot of that work so as to refresh the version in the JDK mainline. As of now the mainline that corresponds to JDK 10.
>>>>
>>>> While incubating in JDK 9, the implementation has been almost completely rewritten over in the sandbox. The implementation is now complete asynchronous ( previously HTTP/1.1 has a blocking implementation ). Use of the RX Flow concept been pushed down into the implementation. This eliminates much of the original custom concepts to support HTTP/2. The "flow" of data can now be more easily traced from the user-level request publishers and response subscribers, all the way down to the underlying socket. This significantly reduces the number of concepts and complexity in the code, and maximizes the possibility of reuse between HTTP/1.1 and HTTP/2.
>>>>
>>>> As the API is still incubating, there have been some API tweaks. Mainly renaming ( request & response body processors are now request publisher and response subscriber ), minor spec clarifications around exceptions, a general tidy up and changes to address a number of external feedback items. Additionally, much work has been done on increasing test coverage and stabilization of the tests themselves.
>>>>
>>>> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>>>>
>>>> The webrev contains contributions from Chris Hegarty, Daniel Fuchs Michael McMahon, and Pavel Rappo. As can be seen from the sandbox history, review of the code has been effectively ongoing as the code has evolved, but nonetheless I'm sure it will benefit from further review.
>>>>
>>>> -Chris.
>>>>
>>>> [1] hg clone http://hg.openjdk.java.net/jdk/sandbox; cd sandbox; hg update http-client-branch
>>>> [2] http://hg.openjdk.java.net/jdk10/sandbox/
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8191494: Refresh the incubating Http Client

Michael McMahon
In reply to this post by Chris Hegarty
Looks good Chris.

- Michael.

On 24/11/2017, 17:05, Chris Hegarty wrote:

> Just an update on this.
>
> There have been many review comments, off line, that have resulted in
> changes pushed to the sandbox, so I've refreshed the webrev at the
> same location.
>
> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>
> -Chris.
>
> On 17/11/17 18:31, Chris Hegarty wrote:
>> Work on the incubating HTTP Client has been ongoing in the
>> `http-client-branch` of the JDK sandbox [1] ( and previously in the
>> JDK 10 sandbox [2] ). This issue proposes to take a snapshot of that
>> work so as to refresh the version in the JDK mainline. As of now the
>> mainline that corresponds to JDK 10.
>>
>> While incubating in JDK 9, the implementation has been almost
>> completely rewritten over in the sandbox. The implementation is now
>> complete asynchronous ( previously HTTP/1.1 has a blocking
>> implementation ). Use of the RX Flow concept been pushed down into
>> the implementation. This eliminates much of the original custom
>> concepts to support HTTP/2. The "flow" of data can now be more easily
>> traced from the user-level request publishers and response
>> subscribers, all the way down to the underlying socket. This
>> significantly reduces the number of concepts and complexity in the
>> code, and maximizes the possibility of reuse between HTTP/1.1 and
>> HTTP/2.
>>
>> As the API is still incubating, there have been some API tweaks.
>> Mainly renaming ( request & response body processors are now request
>> publisher and response subscriber ), minor spec clarifications around
>> exceptions, a general tidy up and changes to address a number of
>> external feedback items. Additionally, much work has been done on
>> increasing test coverage and stabilization of the tests themselves.
>>
>> http://cr.openjdk.java.net/~chegar/http_client_sandbox_8191494/
>>
>> The webrev contains contributions from Chris Hegarty, Daniel Fuchs
>> Michael McMahon, and Pavel Rappo. As can be seen from the sandbox
>> history, review of the code has been effectively ongoing as the code
>> has evolved, but nonetheless I'm sure it will benefit from further
>> review.
>>
>> -Chris.
>>
>> [1] hg clone http://hg.openjdk.java.net/jdk/sandbox; cd sandbox; hg
>> update http-client-branch
>> [2] http://hg.openjdk.java.net/jdk10/sandbox/