Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

Hi Michael et al. I've thought more about the API in general, and have some specific feedback as well. I've also made the Javadoc available of an API which incorporates all my ideas: http://jep110-anthonyvdotbe.rhcloud.com/api/ First of all, some practical questions: Now that the

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

Hi Anthony, Thanks for the feedback. Some of the implementation issues you found have been addressed already, but many have not. So, this is very useful. You can always get the latest version of the source from the sandbox forest and given that it's changing quite rapidly, I will just

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

Hi Michael Here's my feedback. If that'd be helpful, I'm willing to contribute patches to address these points. Either way, please let me know if you have any questions. Some general proposals: First, MultiExchange implements a retry mechanism. However, I see some issues with this: - it's

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

Thanks Roger. Some good catches there. I've added the suggested spec changes (NPE clarifications) to my list that will be dealt with later. On the RedirectFilter comment, the exception is only thrown in the case where the Location header is not present. So, there isn't any further information

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

Hi Michael, A few comments, nothing severe. Several properties are used in the implementation; is it significant that some are sun.net and others java.net? For the new package can we get away from using the "sun." prefix? Exchange.java: 89 in cancel(), exchImpl may be null since it is not

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

> On 18 Feb 2016, at 17:34, Michael McMahon > wrote: > > On 18/02/16 16:06, Paul Sandoz wrote: >>> On 18 Feb 2016, at 16:37, Michael McMahon >>> wrote: >>> When building a request how does one set multiple values for a header?

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

On 18/02/16 16:06, Paul Sandoz wrote: On 18 Feb 2016, at 16:37, Michael McMahon wrote: When building a request how does one set multiple values for a header? setHeaders overwrites, does one used headers(…) instead? headers(String, String) and headers(String

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

> On 18 Feb 2016, at 16:37, Michael McMahon > wrote: > >> When building a request how does one set multiple values for a header? >> setHeaders overwrites, does one used headers(…) instead? >> > > headers(String, String) > and headers(String ...) both accumulate

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

Hi Paul Thanks for the review. Some comments below On 17/02/16 15:26, Paul Sandoz wrote: On 4 Feb 2016, at 17:14, Michael McMahon > wrote: Hi, The following webrevs are for the initial implementation of JEP 110. Most of

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

Hi Michael, Looks great! With a new package, the doc will be important to help get developers started. The package-info.java should help a bit more getting started. The examples in the HttpRequest are very informative, either the package-java should show an example or be more explicit that

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

> On 4 Feb 2016, at 17:14, Michael McMahon wrote: > > Hi, > > The following webrevs are for the initial implementation of JEP 110. > Most of it is in the jdk repository with some build configuration in the top > level repo for putting this code in its own module

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

Michael, just a comment from the supportability side of things. I think the exception handling can be improved with better exception messages which convey state. e.g

RFR: 8087112 HTTP API and HTTP/1.1 implementation

Hi, The following webrevs are for the initial implementation of JEP 110. Most of it is in the jdk repository with some build configuration in the top level repo for putting this code in its own module (java.httpclient). http://cr.openjdk.java.net/~michaelm/8087112/01/top/webrev/

Re: RFR: 8087112 HTTP API and HTTP/1.1 implementation

On 04/02/16 16:14, Michael McMahon wrote: Hi, The following webrevs are for the initial implementation of JEP 110. Most of it is in the jdk repository with some build configuration in the top level repo for putting this code in its own module (java.httpclient).

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

Hi, A new revision of the API can be seen at: http://cr.openjdk.java.net/~michaelm/8087112/04/ The main changes are: - package name change. Moved to own package java.net.httpclient - proxy setting uses ProxySelector (a static factory method will be added to ProxySelector for setting a

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

On 30/09/15 17:34, Michael McMahon wrote: Hi, A new revision of the API can be seen at: http://cr.openjdk.java.net/~michaelm/8087112/04/ The main changes are: - package name change. Moved to own package java.net.httpclient - proxy setting uses ProxySelector (a static factory method will be

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

Anthony, Thanks for the feedback. I am currently looking at API issues but will incorporate these changes when I get back to the implementation. - Michael On 27/09/15 09:14, Anthony Vanelverdinghe wrote: Hi Michael I forgot to mention that, in Utils.java [1], the exception should also be

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

On 27/09/15 18:19, Simone Bordet wrote: Hi, On Fri, Aug 28, 2015 at 10:50 PM, Michael McMahon wrote: Hi, I'm looking for reviewers for the http client implementation. This is just the common API layer and the http/1.1 implementation. The webrev is at

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

Hi, On Mon, Sep 28, 2015 at 10:45 AM, Michael McMahon wrote: > The API has already been approved and the first version to > be integrated won't have all suggestions incorporated. > But, it should be possible to address many of them in time > for JDK 9 anyway. Okay.

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

On 28/09/15 10:13, Simone Bordet wrote: Hi, On Mon, Sep 28, 2015 at 10:45 AM, Michael McMahon wrote: The API has already been approved and the first version to be integrated won't have all suggestions incorporated. But, it should be possible to address many of

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

Hi, On Mon, Sep 28, 2015 at 11:38 AM, Michael McMahon wrote: > if the units are ByteBuffers say, then how do you convert a notification > of n units into a HTTP/2 window update (which has to be in bytes)? The implementation always has this information because it's

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

On 28/09/15 11:23, Simone Bordet wrote: Hi, On Mon, Sep 28, 2015 at 11:38 AM, Michael McMahon wrote: if the units are ByteBuffers say, then how do you convert a notification of n units into a HTTP/2 window update (which has to be in bytes)? The implementation

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

Failed to proof-read. I meant "there is *no* timeout for async calls either" below. Thanks ! On Mon, Sep 28, 2015 at 3:08 PM, Simone Bordet wrote: > Hi, > > On Mon, Sep 28, 2015 at 2:50 PM, Michael McMahon > wrote: >> We might reconsider

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

Hi, On Mon, Sep 28, 2015 at 2:50 PM, Michael McMahon wrote: > We might reconsider this. Currently there is no timeout for blocking > calls, which I guess is problematic. Just to be 100% clear on this, there is timeout for async calls either. The only option is to

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

Hi, On Fri, Aug 28, 2015 at 10:50 PM, Michael McMahon wrote: > Hi, > > I'm looking for reviewers for the http client implementation. > This is just the common API layer and the http/1.1 implementation. > > The webrev is at

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

Hi Michael I forgot to mention that, in Utils.java [1], the exception should also be thrown in case token is the empty string. And while I'm nitpicking anyway: here 's an alternative implementation which extracts the comparison logic in a separate method: static void validateToken(String

Re: RFR: 8087112: HTTP API and HTTP/1.1 implementation

Hi Michael Maybe these have been fixed in the meantime, but I think I've found a bug: in http://cr.openjdk.java.net/~michaelm/8087112/01/src/java.base/share/classes/sun/net/httpclient/Utils.java.html single quotes must be accepted, while parentheses must be rejected [1] and a few typos: in

RFR: 8087112: HTTP API and HTTP/1.1 implementation

Hi, I'm looking for reviewers for the http client implementation. This is just the common API layer and the http/1.1 implementation. The webrev is at http://cr.openjdk.java.net/~michaelm/8087112/01/ Below is a description of the API classes and the relationship with their implementation