Thanks for the review Daniel. > On 28 Nov 2017, at 18:22, Daniel Fuchs <daniel.fu...@oracle.com> 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/ >
