Re: RFR: 8230526: jdk.internal.net.http.PlainProxyConnection is never reused by HttpClient
Daniel, > On 6 Sep 2019, at 14:27, Daniel Fuchs wrote: > > Hi, > > Please find below a fix for: > 8230526: jdk.internal.net.http.PlainProxyConnection is never reused > by HttpClient > https://bugs.openjdk.java.net/browse/JDK-8230526 > > webrev: > http://cr.openjdk.java.net/~dfuchs/webrev_8230526/webrev.00/ The source changes look good. A few small comments on the test: 1) @modules java.net.http This should be jdk.httpserver, right? - since there is a TEST.properties that already specifies the java.net.http module. 2) Maybe refactor some of the initial code in the `test` method into, say, `sanity`? as it is not really testing the bug, but the test itself, right? 3) Trivially, I don’t think that the @library is needed? 4) Maybe reword the comment on the `createHttpsServer` test method, “For convenience the server is used as ...”. -Chris.
Re: RFR: 8230526: jdk.internal.net.http.PlainProxyConnection is never reused by HttpClient
Thanks Chris. All good comments. new webrev at: http://cr.openjdk.java.net/~dfuchs/webrev_8230526/webrev.01/ I'm not sure all httpclient tests that use jdk.httpserver have the appropriate @module tag. But that should be for some further cleanup. best regards, -- daniel On 09/09/2019 16:35, Chris Hegarty wrote: The source changes look good. A few small comments on the test: 1) @modules java.net.http This should be jdk.httpserver, right? - since there is a TEST.properties that already specifies the java.net.http module. 2) Maybe refactor some of the initial code in the `test` method into, say, `sanity`? as it is not really testing the bug, but the test itself, right? 3) Trivially, I don’t think that the @library is needed? 4) Maybe reword the comment on the `createHttpsServer` test method, “For convenience the server is used as ...”.
Re: RFR: 8230526: jdk.internal.net.http.PlainProxyConnection is never reused by HttpClient
> On 9 Sep 2019, at 17:37, Daniel Fuchs wrote: > > Thanks Chris. > > All good comments. > > new webrev at: > http://cr.openjdk.java.net/~dfuchs/webrev_8230526/webrev.01/ LGTM. > I'm not sure all httpclient tests that use jdk.httpserver have the > appropriate @module tag. But that should be for some further cleanup. Agreed. -Chris
