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

2016-03-20 Thread Anthony Vanelverdinghe 

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 implementation has been integrated into jdk9/dev, where 
should I look for the latest changes: "dev"? or still the jep-110 branch 
in "sandbox" for now?
What's the status of the API suggestions in my previous mail [1]? How 
can I tell what 's been "considered and accepted/rejected" (& if 
rejected, why) and what's "to be done"?


=== A bug
The simple test case below downloads a large .zip file 2 times, first 
asynchronously, then synchronously. It has 2 issues:
- the asynchronous block prints "263" as the length of the response in 
bytes (even though the .zip file is obviously much larger)
- the synchronous block immediately throws an "IOException: Connection 
closed" when "response()" is invoked


Note that the order of the blocks doesn't matter: if the 
asynchronous/synchronous blocks are swapped, the result is analog:

- the synchronous block prints "263"
- the asynchronous block immediately throws an "IOException: Connection 
closed" when "get()" is invoked on responseAsync


HttpClient client = HttpClient.getDefault();
URI uri = 
URI.create("http://www.java.net/download/java/jdk9/archive/110/binaries/jdk-9_doc-all.zip;);


// asynchronous
CompletableFuture responseAsync = 
client.request(uri).GET().responseAsync();

Thread.sleep(120_000);
int lengthAsync = 
responseAsync.get().body(HttpResponse.asByteArray()).length;

System.out.println(lengthAsync);

// synchronous
HttpResponse response = client.request(uri).GET().response();
Thread.sleep(120_000);
int length = response.body(HttpResponse.asByteArray()).length;
System.out.println(length);

=== BodyProcessors
In my previous message I proposed an alternative for these interfaces. 
While I admit it had its flaws (e.g. the use of SubmissionPublisher), I 
still believe the API should build upon the Flow API.
Related to this: in your response you said "extending Flow.Subscriber 
means you would not be able to leverage existing Subscriber 
implementations directly". What did you mean with this? How does the 
current API allow to leverage existing Subscriber implementations in a 
way that extending Flow.Subscriber wouldn't?


By building upon the Flow API, the API could simply state "give me a 
correct Flow.Subscriber implementation". On the other hand, by not doing 
so, a lot of specification has to be provided/duplicated, and many 
questions are to be answered, e.g.:
- when is each method called, e.g. can onRequestError be called without 
onRequestStart having been invoked first?

- what if any of the methods throws a RuntimeException?
- what if onRequestBodyChunk fills the ByteBuffer on another Thread 
(e.g. by using an AsynchronousFileChannel) & returns false immediately?
- what if the flowController window remains at 0? Will the library set a 
timeout, after which onXxxError will be invoked? Or will it wait 
indefinitely?


Something else I'm not comfortable with, is that the responsibility for 
providing the ByteBuffers is with the library, instead of the 
BodyProcessor itself. Any buggy BodyProcessor implementation which 
accidentally manipulates ByteBuffers after they've been returned to the 
library, will likely cause totally unrelated requests to fail inexplicably.
Or, worst-case scenario: the library fills a ByteBuffer to be provided 
to a HttpResponse.BodyProcessor X, then a buggy/malicious 
HttpRequest.BodyProcessor Y overwrites the data in it (because the 
Buffer had previously been provided to Y for writing part of its request 
body to it), then X reads the ByteBuffer containing Y's data. Result: X 
returns a corrupt result without being aware of it.


Finally, BodyProcessors don't currently have a way to cancel the 
operation once it has started. For example, assume a large .zip file is 
downloaded, but the BodyProcessor detects the file is corrupt as soon as 
it has read the header (not sure if this is actually possible with .zip 
files, but you get the idea). In this case the BodyProcessor should have 
a way to say "I don't care about the remainder of the response, since 
the file is corrupt anyway".


To address the above issues, I'd like to propose the following interfaces:
in HttpRequest:
interface BodyProcessor extends 
Flow.Processor {}


interface BodyHandler {
BodyProcessor apply(URI uri, String method, Http.Version 
version, HttpHeaders headers);

}

in HttpResponse:
interface BodyProcessor extends 
Flow.Processor, Supplier {}


interface BodyHandler {
BodyProcessor apply(int statusCode, Http.Version version, 
HttpHeaders headers);

}

These BodyProcessor interfaces don't have any methods of their own & 
merely extend the Flow interfaces. Any

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

2016-02-22 Thread Michael McMahon 

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 incorporate your feedback myself.

Considering that the API has been approved (initially) I will do the 
initial integration
without changes to the API. There will be changes before JDK 9 goes out. 
So, we

have an opportunity still to update the API.

We spent some time looking at the Flow API, and the conclusion I came to
was that its publish/subscribe model and flow control mechanism should be
accommodated, though not necessarily mandated as part of the API given the
1 to 1 relationship here, whereas Flow being more 1 to N.

I think that by using basically the same type of flow control mechanism 
means

that the Flow API could be used on top of the currently specified API
and SubmissionPublisher could be used in the way you suggest. But,
I don't think we should require all request body processors to use 
SubmissionPublisher

though. That seems way too specific.

On the receiving side, I like the use of Optional there and what you're 
suggesting
is concise and quite similar to what is there functionally. In fact we 
did look at this
option before. But, extending Flow.Subscriber means you would not be 
able to leverage
existing Subscriber implementations directly and unless Flow is being 
used on the sending

side as well, I don't think it is worth it here.

I'll come back to you on the other API suggestions (and implementation 
comments if I have

questions)

Thanks
Michael.

On 22/02/16 00:47, Anthony Vanelverdinghe wrote:

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 unconfigurable and undocumented in the API
- it seems that requests are retried irrespective of the HTTP method. 
Everything that is not a standard, idempotent method should never be 
retried.

- a given HttpRequest.BodyProcessor may not be able to handle retries
Given the above, I believe the retry mechanism should be disabled by 
default for now (it seems that setting DEFAULT_MAX_ATTEMPTS to 0 would 
do so).



Second, I'd like to change the Processor interfaces as below, to make 
use of the j.u.c.Flow API.


interface HttpRequest.BodyProcessor {

void send(HttpRequest request, SubmissionPublisher 
publisher);


}

and an example implementation:

class FromString implements HttpRequest.BodyProcessor {

final String s;

FromString(String s) {
this.s = s;
}

void send(HttpRequest request, SubmissionPublisher 
publisher) {

Optional charset = getCharsetParameterOfContentType();
charset.ifPresentOrElse(
cs -> { publisher.submit(ByteBuffer.wrap(s.getBytes(cs))); 
publisher.close(); },
() -> publisher.closeExceptionally(new 
IllegalArgumentException("request doesn't specify charset"))

);
}

}


interface HttpResponse.BodyProcessor extends 
Flow.Subscriber {


Optional beforeReceipt(long contentLength, int statusCode, 
HttpHeaders responseHeaders) throws IOException;


T afterReceipt() throws IOException;

}

and an example implementation:

class AsFile implements HttpResponse.BodyProcessor {

final Path p;
FileChannel fc;
IOException e;
Flow.Subscription subscription;

AsFile(Path p) {
this.p = p;
}

Optional beforeReceipt(long contentLength, int statusCode, 
HttpHeaders responseHeaders) throws IOException {

this.fc = FileChannel.open(p);
return Optional.empty();
}

Path afterReceipt() throws IOException {
if(e == null) {
return p;
} else {
throw e;
}
}

void onSubscribe(Flow.Subscription subscription) {
this.subscription = subscription;
subscription.request(1);
}

void onNext(ByteBuffer item) {
try {
fc.write(item);
subscription.request(1);
} catch(IOException e) {
subscription.cancel();
try {
fc.close();
} catch(IOException eSup) {
e.addSuppressed(eSup);
}
this.e = e;
}
}

void onComplete() {
try {
fc.close();
} catch(IOException e) {
this.e = e;
}
}

void onError(Throwable throwable) {
try {
fc.close();
} catch(IOException e) {
this.e = e;
}
}

}


Finally, HttpResponse.MultiProcessor would be eliminated altogether, 
by replacing HttpRequest::multiResponseAsync with the following method:
CompletionStage

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

2016-02-21 Thread Anthony Vanelverdinghe 

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 unconfigurable and undocumented in the API
- it seems that requests are retried irrespective of the HTTP method. 
Everything that is not a standard, idempotent method should never be 
retried.

- a given HttpRequest.BodyProcessor may not be able to handle retries
Given the above, I believe the retry mechanism should be disabled by 
default for now (it seems that setting DEFAULT_MAX_ATTEMPTS to 0 would 
do so).



Second, I'd like to change the Processor interfaces as below, to make 
use of the j.u.c.Flow API.


interface HttpRequest.BodyProcessor {

void send(HttpRequest request, SubmissionPublisher 
publisher);


}

and an example implementation:

class FromString implements HttpRequest.BodyProcessor {

final String s;

FromString(String s) {
this.s = s;
}

void send(HttpRequest request, SubmissionPublisher 
publisher) {

Optional charset = getCharsetParameterOfContentType();
charset.ifPresentOrElse(
cs -> { publisher.submit(ByteBuffer.wrap(s.getBytes(cs))); 
publisher.close(); },
() -> publisher.closeExceptionally(new 
IllegalArgumentException("request doesn't specify charset"))

);
}

}


interface HttpResponse.BodyProcessor extends 
Flow.Subscriber {


Optional beforeReceipt(long contentLength, int statusCode, 
HttpHeaders responseHeaders) throws IOException;


T afterReceipt() throws IOException;

}

and an example implementation:

class AsFile implements HttpResponse.BodyProcessor {

final Path p;
FileChannel fc;
IOException e;
Flow.Subscription subscription;

AsFile(Path p) {
this.p = p;
}

Optional beforeReceipt(long contentLength, int statusCode, 
HttpHeaders responseHeaders) throws IOException {

this.fc = FileChannel.open(p);
return Optional.empty();
}

Path afterReceipt() throws IOException {
if(e == null) {
return p;
} else {
throw e;
}
}

void onSubscribe(Flow.Subscription subscription) {
this.subscription = subscription;
subscription.request(1);
}

void onNext(ByteBuffer item) {
try {
fc.write(item);
subscription.request(1);
} catch(IOException e) {
subscription.cancel();
try {
fc.close();
} catch(IOException eSup) {
e.addSuppressed(eSup);
}
this.e = e;
}
}

void onComplete() {
try {
fc.close();
} catch(IOException e) {
this.e = e;
}
}

void onError(Throwable throwable) {
try {
fc.close();
} catch(IOException e) {
this.e = e;
}
}

}


Finally, HttpResponse.MultiProcessor would be eliminated altogether, by 
replacing HttpRequest::multiResponseAsync with the following method:
CompletionStage> 
responseAsync(BiFunction pushHandler) 
{ ... }


Moreover, response() and responseAsync() could then easily be 
implemented in terms of this method, as follows:

HttpResponse response() { return responseAsync().get(); }
CompletionStage responseAsync() { return 
responseAsync((request, headers) -> false).getKey(); }



On to more specific issues:

On the Javadoc:
java.net.http package-info
- "superceded" should be "superseded"


On the API:
java.net.http
- convert all classes to interfaces
--- none of the classes contains any implementation code
- specify NPEs wherever applicable
- specify synchronization requirements more rigorously
--- e.g. should the ProxySelector of a HttpClient be thread-safe, or 
will the HttpClient synchronize access externally (assuming the 
ProxySelector is confined to the HttpClient)?


HttpClient
- add "Optional timeout()"
- authenticator(): specify that if no Authenticator was set, but a 
default was set with Authenticator.setDefault(), then that Authenticator 
will be used (though not returned from this method)

--- ease migration of java.net-based applications
- cookieManager(): change to "CookieHandler cookieHandler()", with 
default value CookieHandler.getDefault() if set, or else a default 
object is returned (e.g. new CookieManager())
--- allows to use CookieHandler.getDefault() and ease migration of 
java.net-based applications

- debugPrint(): remove
- followRedirects(): change to "Predicate redirectionPolicy()"
--- allows to implement custom redirection policies
- pipelining(): remove
--- unsupported
- proxySelector(): change to "ProxySelector proxySelector()", with 
default value ProxySelector.getDefault()

--- use a sensible default

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

2016-02-19 Thread Michael McMahon 

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 available to be logged.

- Michael

On 18/02/16 17:02, Roger Riggs wrote:

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 
initialized by the constructor


Methods like responseAsyncImpl0 should be private unless they are 
needed outside the class.
It might help maintainability over time as different maintainers need 
to understand the scope

of use.

ExecutorWrapper:82 println/printStackTrace!  Perhaps Log or the system 
logger instead


Http1Request: 46 ; a comment about which buffer indices are used for 
what might be useful


Http1Request:172: finished() is synchronized but the assignment = true 
(line 166 and 232) is not.

The field 'finished' may be hidden by 'finished' the local:  Line 255.

Unused final static fields:  CRLF_SIZE, CRLF_BUFFER, 
EMPTY_CHUNK_HEADER_SIZE.


Http1Response:109  'finished' field should be private. (and probably 
others)


HttpClientBuilderImpl:53  spec for cookieManager says it should be 
non-null;  add Objects.requireNonNull...

Ditto sslContext, sslParameters, executor, followRedirects, etc.


HttpClientImpl: 167 obsolete comment about ManagedLocalsThread (and no 
explicit call to super() to inhibit inherited thread locals; if that's 
needed)



HttpRequest.Builder does not prohibit supplying null to its methods.
Is it intentional to be able to supply null and get/revert to the 
default behavior.

If so, it should be specified.  Or the opposite requiring non-null.
HttpRequestBuilderImpl.java would be affected if the later.

HttpRequestBuilderImpl: 96;  should copy() be doing a deep copy of the 
User Headers?
Otherwise, subsequent changes to either HttpRequestBuilder would 
affect the other.


RedirectFilter: 85:  Invalid redirection exception should include the 
invalid value for debug.


That's it for now,

Roger


On 2/4/16 11:14 AM, 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).

http://cr.openjdk.java.net/~michaelm/8087112/01/top/webrev/

http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/

The HTTP/2 implementation will come later.

Thanks,
Michael.

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

2016-02-18 Thread Roger Riggs 

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 
initialized by the constructor


Methods like responseAsyncImpl0 should be private unless they are needed 
outside the class.
It might help maintainability over time as different maintainers need to 
understand the scope

of use.

ExecutorWrapper:82 println/printStackTrace!  Perhaps Log or the system 
logger instead


Http1Request: 46 ; a comment about which buffer indices are used for 
what might be useful


Http1Request:172: finished() is synchronized but the assignment = true 
(line 166 and 232) is not.

The field 'finished' may be hidden by 'finished' the local:  Line 255.

Unused final static fields:  CRLF_SIZE, CRLF_BUFFER, 
EMPTY_CHUNK_HEADER_SIZE.


Http1Response:109  'finished' field should be private. (and probably others)

HttpClientBuilderImpl:53  spec for cookieManager says it should be 
non-null;  add Objects.requireNonNull...

Ditto sslContext, sslParameters, executor, followRedirects, etc.


HttpClientImpl: 167 obsolete comment about ManagedLocalsThread (and no 
explicit call to super() to inhibit inherited thread locals; if that's 
needed)



HttpRequest.Builder does not prohibit supplying null to its methods.
Is it intentional to be able to supply null and get/revert to the 
default behavior.

If so, it should be specified.  Or the opposite requiring non-null.
HttpRequestBuilderImpl.java would be affected if the later.

HttpRequestBuilderImpl: 96;  should copy() be doing a deep copy of the 
User Headers?
Otherwise, subsequent changes to either HttpRequestBuilder would affect 
the other.


RedirectFilter: 85:  Invalid redirection exception should include the 
invalid value for debug.


That's it for now,

Roger


On 2/4/16 11:14 AM, 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).

http://cr.openjdk.java.net/~michaelm/8087112/01/top/webrev/

http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/

The HTTP/2 implementation will come later.

Thanks,
Michael.

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

2016-02-18 Thread Paul Sandoz 

> 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? 
 setHeaders overwrites, does one used headers(…) instead?
 
>>> headers(String, String)
>>> and headers(String ...) both accumulate headers
>>> 
>> What headers would be included in the request for the following expressions:
>> 
>> -  setHeader(“foo”, “value1”).setHeaders(“foo”, “value2”)
> 
> Foo: value2
>> -  headers(“foo”, “value2”).headers(“foo”, “value2”)
> 
> Foo: value2
> Foo: value2
> 

Ok, that is what i thought, unless i missed it in the docs we should document 
that more clearly.


 OK, i see there is more specification in HttpResponse.BodyProcessor. I 
 wonder how implementations correlate the unit to bytes? Is there any 
 correlation related to an internal buffer size? Why would the requested 
 unfulfilled demand be greater than 1? i.e. as an implementor of a body 
 processor how am i meant to correlate the bytes i read/write to the units 
 required to express the unfulfilled demand?
 
>>> There is no direct correlation between units and bytes. If the window size 
>>> is 1, then the processor
>>> is willing to accept one more call, of whatever sized ByteBuffer.
>>> 
>>> The implementor of the processor must then decide based on how long it 
>>> takes to consume the
>>> data whether to update the window immediately or to delay and update it 
>>> later.
>>> 
>> Sorry, still don’t get why a value greater than 1 needs to be provided. If i 
>> pass in 1 and may get one or more calls, then why pass in 1? Why not use a 
>> Runnable instead?
>> 
> 
> In practice, 1 seems to be the most likely use-case, but a value of N means
> that N further calls to provide 1 ByteBuffer is permitted.
> 
> It's exactly the same with the Flow API.
> 

It does not seem quite so because there is a correlation between the publisher 
and subscriber in terms of elements, where N in request(N) represents the
additional unfulfilled demand to process elements that are published.

In your case the process implementation has no way to correlate the demand in a 
meaningful way.

If there was a correlation to bytes then it may be more meaningful. For example 
flowConsumer.accept(4096) means the processor is requesting an additional 
unfulfilled demand of 4096 bytes, that way the processor can control the rate 
in terms of something it understands and it’s up to the HTTP client to respect 
that (i dunno if we can get down to the traffic control level of the TCP 
protocol in Java).

I really don’t wanna block this, but this seems to require some more bake time. 
If you can I recommend removing it from the API until you revise with HTTP 2. 
If you think this would be more work than necessary, then lets just promise to 
revise when HTTP 2 goes in.


 For responses, what happens if the Consumer is called outside the 
 life-cycle of the processors?
 
>>> Actually, that is not part of the current API. For the moment, the 
>>> life-cycle of the ByteBuffer is
>>> that it only belongs to the processor for the duration of the call, meaning 
>>> it would have to be
>>> copied if the data isn't consumed fully during the call.
>>> 
>> But happens if the Consumer is invoked outside of the life-cycle of the 
>> response, should an ISE be thrown?
>> 
>> Same for the LongConsumer, what happens if it is invoked outside the 
>> life-cycle of the request or response.
> 
> There is no Consumer<>

Here is the API signature:

+T onResponseBodyStart(long contentLength,
+  HttpHeaders responseHeaders,
+  LongConsumer flowController,
+  Consumer bufferpool)
+throws IOException;

There is Consumer as well as LongConsumer.


> but for the LongConsumer, it's not specified. I would expect it just to have
> no effect.
> 

We need to document the behaviour for both consumers.

Paul.


signature.asc
Description: Message signed with OpenPGP using GPGMail

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

2016-02-18 Thread Michael McMahon 

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 ...) both accumulate headers


What headers would be included in the request for the following expressions:

-  setHeader(“foo”, “value1”).setHeaders(“foo”, “value2”)


Foo: value2

-  headers(“foo”, “value2”).headers(“foo”, “value2”)


Foo: value2
Foo: value2


Or put another way if i want my request to contain the header lines:

   Foo: value1
   Foo: value2

how can i express that in the API?



Processors
—

I think the flowController of HttpRequest.BodyProcessor.onRequestStart requires 
more specification (a follow up issue if you wish):
- what are the units? It represents unfulfilled demand, but of what?
- what if a -ve value is passed
- what if Long.MAX_VALUE is passed? effectively unbounded?
- does it have to be called within onRequestStart and onRequestBodyChunk?
- what happens if it is invoked outside the life-cycle of a BodyProcessor?


yes, spec that is in HttpResponse.BodyProcessor applies here too,
but it should be repeated.


Ok.



OK, i see there is more specification in HttpResponse.BodyProcessor. I wonder 
how implementations correlate the unit to bytes? Is there any correlation 
related to an internal buffer size? Why would the requested unfulfilled demand 
be greater than 1? i.e. as an implementor of a body processor how am i meant to 
correlate the bytes i read/write to the units required to express the 
unfulfilled demand?


There is no direct correlation between units and bytes. If the window size is 
1, then the processor
is willing to accept one more call, of whatever sized ByteBuffer.

The implementor of the processor must then decide based on how long it takes to 
consume the
data whether to update the window immediately or to delay and update it later.


Sorry, still don’t get why a value greater than 1 needs to be provided. If i 
pass in 1 and may get one or more calls, then why pass in 1? Why not use a 
Runnable instead?



In practice, 1 seems to be the most likely use-case, but a value of N means
that N further calls to provide 1 ByteBuffer is permitted.

It's exactly the same with the Flow API.


For responses, what happens if the Consumer is called outside the 
life-cycle of the processors?


Actually, that is not part of the current API. For the moment, the life-cycle 
of the ByteBuffer is
that it only belongs to the processor for the duration of the call, meaning it 
would have to be
copied if the data isn't consumed fully during the call.


But happens if the Consumer is invoked outside of the life-cycle of the 
response, should an ISE be thrown?

Same for the LongConsumer, what happens if it is invoked outside the life-cycle 
of the request or response.


There is no Consumer<> but for the LongConsumer, it's not specified. I 
would expect it just to have

no effect.

Michael

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

2016-02-18 Thread Paul Sandoz 

> 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 headers
> 

What headers would be included in the request for the following expressions:

-  setHeader(“foo”, “value1”).setHeaders(“foo”, “value2”)

-  headers(“foo”, “value2”).headers(“foo”, “value2”)

Or put another way if i want my request to contain the header lines:

  Foo: value1
  Foo: value2

how can i express that in the API?


>> 
>> Processors
>> —
>> 
>> I think the flowController of HttpRequest.BodyProcessor.onRequestStart 
>> requires more specification (a follow up issue if you wish):
>> - what are the units? It represents unfulfilled demand, but of what?
>> - what if a -ve value is passed
>> - what if Long.MAX_VALUE is passed? effectively unbounded?
>> - does it have to be called within onRequestStart and onRequestBodyChunk?
>> - what happens if it is invoked outside the life-cycle of a BodyProcessor?
>> 
> 
> yes, spec that is in HttpResponse.BodyProcessor applies here too,
> but it should be repeated.
> 

Ok.


>> OK, i see there is more specification in HttpResponse.BodyProcessor. I 
>> wonder how implementations correlate the unit to bytes? Is there any 
>> correlation related to an internal buffer size? Why would the requested 
>> unfulfilled demand be greater than 1? i.e. as an implementor of a body 
>> processor how am i meant to correlate the bytes i read/write to the units 
>> required to express the unfulfilled demand?
>> 
> There is no direct correlation between units and bytes. If the window size is 
> 1, then the processor
> is willing to accept one more call, of whatever sized ByteBuffer.
> 
> The implementor of the processor must then decide based on how long it takes 
> to consume the
> data whether to update the window immediately or to delay and update it later.
> 

Sorry, still don’t get why a value greater than 1 needs to be provided. If i 
pass in 1 and may get one or more calls, then why pass in 1? Why not use a 
Runnable instead?


>> For responses, what happens if the Consumer is called outside the 
>> life-cycle of the processors?
>> 
> 
> Actually, that is not part of the current API. For the moment, the life-cycle 
> of the ByteBuffer is
> that it only belongs to the processor for the duration of the call, meaning 
> it would have to be
> copied if the data isn't consumed fully during the call.
> 

But happens if the Consumer is invoked outside of the life-cycle of the 
response, should an ISE be thrown?

Same for the LongConsumer, what happens if it is invoked outside the life-cycle 
of the request or response.

Paul.


signature.asc
Description: Message signed with OpenPGP using GPGMail

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

2016-02-18 Thread Michael McMahon 

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 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/ 



http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/



I am mostly focusing on the API, because that is where i have had most 
input in the past. Over all the API is quite compact, appears easy to 
use for common cases, yet has a powerful plugin mechanism for 
request/response bodies, and the design allows for but the 
implementation does not have to provide a fully non-blocking 
implementation.



HttpHeaders
—

   61 public Optional firstValueAsLong(String name);
Why not return OptionalLong?



Okay. I've added this to JDK-8134652 which is tracking small
spec. changes.

The implementation class HttpHeadersImpl does not appear to 
support unmodifiable values i.e. the list obtained from the header map 
can be modified. Also might need to be careful about clone, since that 
can be called by the user, and this is a shallow clone.



Right. The map and any component lists should be unmodifiable.
Will fix this.



HttpRequest
—

In the examples recommend using URI.create rather than new URI.

The examples section can be placed under an @apiNote



ok, will add this for spec change also

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 headers



Processors
—

I think the flowController of HttpRequest.BodyProcessor.onRequestStart 
requires more specification (a follow up issue if you wish):

- what are the units? It represents unfulfilled demand, but of what?
- what if a -ve value is passed
- what if Long.MAX_VALUE is passed? effectively unbounded?
- does it have to be called within onRequestStart and onRequestBodyChunk?
- what happens if it is invoked outside the life-cycle of a BodyProcessor?


yes, spec that is in HttpResponse.BodyProcessor applies here too,
but it should be repeated.

OK, i see there is more specification in HttpResponse.BodyProcessor. I 
wonder how implementations correlate the unit to bytes? Is there any 
correlation related to an internal buffer size? Why would the 
requested unfulfilled demand be greater than 1? i.e. as an implementor 
of a body processor how am i meant to correlate the bytes i read/write 
to the units required to express the unfulfilled demand?


There is no direct correlation between units and bytes. If the window 
size is 1, then the processor

is willing to accept one more call, of whatever sized ByteBuffer.

The implementor of the processor must then decide based on how long it 
takes to consume the
data whether to update the window immediately or to delay and update it 
later.


For responses, what happens if the Consumer is called outside 
the life-cycle of the processors?




Actually, that is not part of the current API. For the moment, the 
life-cycle of the ByteBuffer is
that it only belongs to the processor for the duration of the call, 
meaning it would have to be

copied if the data isn't consumed fully during the call.

Thanks!

Michael

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

2016-02-17 Thread Roger Riggs 

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 HttpRequest is the place to start.

RequestProcessor: +126  Unclosed {@code ...
The generated javadoc comes out with visible {@link...

java/net/package-info.java:  "This API has been superseded by the newer 
HTTP client API described in the previous section."

But there is no description in a previous section.


HttpClient has a debugPrint method without a description; it may be a 
leftover from development and should be removed.


HttpClient.Builder should specify the default behavior if 
authenticator(a) is not called.

Ditto cookieManager(m).


A general statement in each class or package about throwing NPE if a 
parameter is null unless

otherwise specified might be cleaner than individual @throws clauses

RequestProcessor.onRequestStart:  there is an @returns TODO that seems 
to have been described above.



HttpResponse.BodyProcessor: If the flowController.accept is never 
called, will it hang indefinitely
or will the timeout eventually force an error.  An inadvertent 
programming error (omission) should not

hang forever without any indication.

bufferpool:  if the consumer bufferpool is never called, what happens to 
the buffers?  Are they GC'ed or leak?


The description of onResponseBodyStart returning a T that will read the 
body is a bit vague.
I'm not sure what methods on T would be used to read the body or who 
would call them.


None of these are show stoppers, but perhaps the easy ones can be done 
before the push.


Tomorrow, I'll look more closely at the implementation.

Roger



On 2/4/2016 11:14 AM, 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).

http://cr.openjdk.java.net/~michaelm/8087112/01/top/webrev/

http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/

The HTTP/2 implementation will come later.

Thanks,
Michael.

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

2016-02-17 Thread Paul Sandoz 

> 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 (java.httpclient).
> 
> http://cr.openjdk.java.net/~michaelm/8087112/01/top/webrev/
> 
> http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/
> 

I am mostly focusing on the API, because that is where i have had most input in 
the past. Over all the API is quite compact, appears easy to use for common 
cases, yet has a powerful plugin mechanism for request/response bodies, and the 
design allows for but the implementation does not have to provide a fully 
non-blocking implementation.


HttpHeaders
—

  61 public Optional firstValueAsLong(String name);
Why not return OptionalLong?

The implementation class HttpHeadersImpl does not appear to support 
unmodifiable values i.e. the list obtained from the header map can be modified. 
Also might need to be careful about clone, since that can be called by the 
user, and this is a shallow clone.


HttpRequest
—

In the examples recommend using URI.create rather than new URI.

The examples section can be placed under an @apiNote

When building a request how does one set multiple values for a header? 
setHeaders overwrites, does one used headers(…) instead?


Processors
—

I think the flowController of HttpRequest.BodyProcessor.onRequestStart requires 
more specification (a follow up issue if you wish):
- what are the units? It represents unfulfilled demand, but of what?
- what if a -ve value is passed
- what if Long.MAX_VALUE is passed? effectively unbounded?
- does it have to be called within onRequestStart and onRequestBodyChunk?
- what happens if it is invoked outside the life-cycle of a BodyProcessor?

OK, i see there is more specification in HttpResponse.BodyProcessor. I wonder 
how implementations correlate the unit to bytes? Is there any correlation 
related to an internal buffer size? Why would the requested unfulfilled demand 
be greater than 1? i.e. as an implementor of a body processor how am i meant to 
correlate the bytes i read/write to the units required to express the 
unfulfilled demand?

For responses, what happens if the Consumer is called outside the 
life-cycle of the processors?

Paul.


> The HTTP/2 implementation will come later.
> 
> Thanks,
> Michael.



signature.asc
Description: Message signed with OpenPGP using GPGMail

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

2016-02-05 Thread Sean Coffey 

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
http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/src/java.httpclient/share/classes/java/net/http/Http1Request.java.html
 319 if (contentWritten != contentLength) {
 320 throw new IOException("wrong content length");

Let's print the length value.

 
http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/src/java.httpclient/share/classes/java/net/http/AuthenticationFilter.java.html
 215 } else if (au.retries > retry_limit) {
 216 throw new IOException("too many authentication attempts");

Let's print the retry limit.

http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/src/java.httpclient/share/classes/java/net/http/Http1Response.java.html
  90 } else {
  91 throw new IOException("Unexpected HTTP protocol version");

Let's print the statusline value in such cases.

These are just some examples. I can help contribute a patch if you want 
or perhaps you can run through and edit in your own repo.


regards,
Sean.

On 04/02/2016 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).

http://cr.openjdk.java.net/~michaelm/8087112/01/top/webrev/

http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/

The HTTP/2 implementation will come later.

Thanks,
Michael.

RFR: 8087112 HTTP API and HTTP/1.1 implementation

2016-02-04 Thread Michael McMahon 

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/

http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/

The HTTP/2 implementation will come later.

Thanks,
Michael.

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

2016-02-04 Thread Chris Hegarty 

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).

http://cr.openjdk.java.net/~michaelm/8087112/01/top/webrev/

http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/


Great to see this moving forward Michael, and I think having it in
its own module makes sense. As suggested by the changeset comment
in the webrevs, I have already reviewed the code and worked with
you to have my comments addressed in the sandbox. I will, however,
take a fresh look at the webrevs, but I am happy to be listed as a
reviewer.


The HTTP/2 implementation will come later.


That sounds fine.

-Chris

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

2015-09-30 Thread Michael McMahon 

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 simple fixed address for all proxies).
  Using ProxySelector gives access to system default proxies.

- HttpResponseBodyProcessor uses flow control based on Flow api.

- miscellaneous doc cleanups/clarifications

Thanks,
Michael

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

2015-09-30 Thread Michael McMahon 

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 added
  to ProxySelector for setting a simple fixed address for all proxies).
  Using ProxySelector gives access to system default proxies.

- HttpResponseBodyProcessor uses flow control based on Flow api.

- miscellaneous doc cleanups/clarifications

Thanks,
Michael


Also, an explicit timeout is added and a TimeoutException thrown
when responses are not received within specified time.

Michael

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

2015-09-29 Thread Michael McMahon 

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 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 token, String errormsg) {
if (token.isEmpty() || token.chars().anyMatch(c -> !isTchar(c))) {
throw new IllegalArgumentException(errormsg);
}
}

private static boolean isTchar(int c) {
return c >= 0x30 && c <= 0x39 // 0 - 9
|| (c >= 0x61 && c <= 0x7a) // a - z
|| (c >= 0x41 && c <= 0x5a) // A - Z
|| (c >= 0x21 && c <= 0x2e && c != 0x22 && c != 0x28 && c 
!= 0x29 && c != 0x2c)

|| (c >= 0x5e && c <= 0x60)
|| (c == 0x7c) || (c == 0x7e);
}

PS: I have signed the OCA [2], so you can reuse any code if you wish

[1] 
http://cr.openjdk.java.net/~michaelm/8087112/01/src/java.base/share/classes/sun/net/httpclient/Utils.java.html

[2] http://www.oracle.com/technetwork/community/oca-486395.html#v

Kind regards,
Anthony


On 25/09/2015 21:59, Anthony Vanelverdinghe wrote:

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 
http://cr.openjdk.java.net/~michaelm/8087112/01/src/java.base/share/classes/sun/net/httpclient/SSLTunnelConnection.java.html

at line 56, replace
return connected & delegate.connected();
with
return connected && delegate.connected();

in 
http://cr.openjdk.java.net/~michaelm/8087112/01/src/java.base/share/classes/java/net/package-info.java.udiff.html

replace "superceded" with "superseded"

On a final note, when will the new API be integrated in the weekly 
early access builds [2]?


[1] https://tools.ietf.org/html/rfc7230#section-3.2.6
[2] https://jdk9.java.net/download/

Kind regards
Anthony

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

2015-09-28 Thread Michael McMahon 

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 http://cr.openjdk.java.net/~michaelm/8087112/01/

It appears that none of my concerns expressed in this message:
http://mail.openjdk.java.net/pipermail/net-dev/2015-July/009048.html
have been addressed.
Am I looking at the right link ?


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.


In particular:

A) lack of backpressure,


My current thoughts on this is to have credit based flow control
window (similar to the Flow API) indicating the number of bytes
that a HttpResponseBodyProcessor is willing to accept and this
maps directly on to the HTTP/2 flow control window and the socket
buffer when doing HTTP/1.


B) missing "request complete" event


When you suggested this, I decided to split the API so that requests
and responses complete independently, but this was too drastic a change
and most use-cases are satisfied by completion of both request and response.

It's also possible to be notified of request completion through the
request processor. It should be straight forward to wrap any request 
processor

in a processor which provides a CompletableFuture for completion of the
request.


C) missing timeouts (CompletableFuture.get(time) is obviously not the
right semantic)


CompletableFuture.get() is at least useful for notifying timeouts.
The question then is what to do next and what was missing was
cancellation, and that has been added. Does that not suffice?

- Michael

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

2015-09-28 Thread Simone Bordet 
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.

>> A) lack of backpressure,
>
> My current thoughts on this is to have credit based flow control
> window (similar to the Flow API) indicating the number of bytes
> that a HttpResponseBodyProcessor is willing to accept and this
> maps directly on to the HTTP/2 flow control window and the socket
> buffer when doing HTTP/1.

Bear in mind that application must have control over this, so you have
to expose an API that application can use to notify that they are done
with the content.
I am not sure the number of bytes is the right unit - my experience is
that you want to trade ByteBuffers as a unit, not the bytes they
contain.
This is also critical for buffer reuse and to avoid nasty surprises on
who owns the buffer.

To be clear:

class MyResponseProcessor implements HttpResponseBodyProcessor
{
void onResponseBodyChunk(ByteBuffer buffer)
{
writeAsyncToDisk(buffer, new CompletionHandler()
{
success: 
failure: 
});
}
}

>> B) missing "request complete" event
>
> When you suggested this, I decided to split the API so that requests
> and responses complete independently, but this was too drastic a change
> and most use-cases are satisfied by completion of both request and response.

I am not sure I follow here.
There is currently no way for an application to be notified of request
completion, so there is no way to be notified of completion of both
request and response, only of response.
Are you saying that most use cases will require this, and so it needs
to be addressed, or are you saying that it is already done in a way
that I could not see ?
Can you expand on this ?

> It's also possible to be notified of request completion through the
> request processor. It should be straight forward to wrap any request
> processor
> in a processor which provides a CompletableFuture for completion of the
> request.

You mean HttpRequestBodyProcessor ?
I don't think it's invoked if there is no body, and it has no
onRequestBodyComplete() method.
Can you expand again ?

>> C) missing timeouts (CompletableFuture.get(time) is obviously not the
>> right semantic)
>
> CompletableFuture.get() is at least useful for notifying timeouts.

There is the need for a completely async timeout.

Plus, the semantic of Future.get() is that the time you specified to
get() has passed.
There is no implied semantic that the request has been aborted, and
aborting the request via cancel() would break the Future.get(time)
contract.

Really, Future.get(time) is the wrong semantic, you don't want to use that.

I suggest that blocking methods throw TimeoutException, and that async
methods get notified with a TimeoutException on their
CompletableFuture.

> The question then is what to do next and what was missing was
> cancellation, and that has been added. Does that not suffice?

Nope. As said above, there is no async timeout, no idle timeout, no
total (request + response) timeout, which are in my experience the 3
most requested features about timeouts.

-- 
Simone Bordet
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz

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

2015-09-28 Thread Michael McMahon 

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 them in time
for JDK 9 anyway.

Okay.


A) lack of backpressure,

My current thoughts on this is to have credit based flow control
window (similar to the Flow API) indicating the number of bytes
that a HttpResponseBodyProcessor is willing to accept and this
maps directly on to the HTTP/2 flow control window and the socket
buffer when doing HTTP/1.

Bear in mind that application must have control over this, so you have
to expose an API that application can use to notify that they are done
with the content.
I am not sure the number of bytes is the right unit - my experience is
that you want to trade ByteBuffers as a unit, not the bytes they
contain.


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)?

Perhaps, if n is required always to be equal to 1, then you could do it
because you would always be referring to the last buffer received
but it seems trickier if n > 1.


This is also critical for buffer reuse and to avoid nasty surprises on
who owns the buffer.

To be clear:

class MyResponseProcessor implements HttpResponseBodyProcessor
{
 void onResponseBodyChunk(ByteBuffer buffer)
 {
 writeAsyncToDisk(buffer, new CompletionHandler()
 {
 success: 
 failure: 
 });
 }
}


B) missing "request complete" event

When you suggested this, I decided to split the API so that requests
and responses complete independently, but this was too drastic a change
and most use-cases are satisfied by completion of both request and response.

I am not sure I follow here.
There is currently no way for an application to be notified of request
completion, so there is no way to be notified of completion of both
request and response, only of response.
Are you saying that most use cases will require this, and so it needs
to be addressed, or are you saying that it is already done in a way
that I could not see ?
Can you expand on this ?


What I was saying was that completion of response also implies
competion of request (ie that it isn't notified until after both happens)


It's also possible to be notified of request completion through the
request processor. It should be straight forward to wrap any request
processor
in a processor which provides a CompletableFuture for completion of the
request.

You mean HttpRequestBodyProcessor ?
I don't think it's invoked if there is no body, and it has no
onRequestBodyComplete() method.
Can you expand again ?


It doesn't have a onComplete() type method. The notification
is implied by the final call to onRequestBodyChunk(). But, I accept
that doesn't actually mean the data has been fully written.
So, maybe an onComplete() method is useful ...

C) missing timeouts (CompletableFuture.get(time) is obviously not the
right semantic)

CompletableFuture.get() is at least useful for notifying timeouts.

There is the need for a completely async timeout.

Plus, the semantic of Future.get() is that the time you specified to
get() has passed.
There is no implied semantic that the request has been aborted, and
aborting the request via cancel() would break the Future.get(time)
contract.


Why would it break the contract? Note, I am referring to
HttpRequest.cancel() not CompletableFuture.cancel()


Really, Future.get(time) is the wrong semantic, you don't want to use that.

I suggest that blocking methods throw TimeoutException, and that async
methods get notified with a TimeoutException on their
CompletableFuture.


The question then is what to do next and what was missing was
cancellation, and that has been added. Does that not suffice?

Nope. As said above, there is no async timeout, no idle timeout, no
total (request + response) timeout, which are in my experience the 3
most requested features about timeouts.

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

2015-09-28 Thread Simone Bordet 
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 the one
that passed the ByteBuffer to the application, so it would be trivial
for the implementation to make this conversion.

> Perhaps, if n is required always to be equal to 1, then you could do it
> because you would always be referring to the last buffer received
> but it seems trickier if n > 1.

Consider onRequestBodyChunk(ByteBuffer) as Flow.Subscriber.onNext(ByteBuffer).
What an application wants to say here is "give me another buffer", not
"give me 42 bytes".

The 42 bytes are irrelevant because the implementation won't certainly
read 42 bytes *only* from the network: it typically has buffers of
certain size that it reuses for network reads.

The application will always be in control if it really needs 42 bytes,
whatever length is the buffer that the implementation passes to it.
It can buffer the bytes if they're less than 42, it can slice the
buffer if they're more than 42, it can do whatever it needs to do to
comply with the 42 bytes requirement.
What it needs from the implementation is just another ByteBuffer.

> What I was saying was that completion of response also implies
> competion of request (ie that it isn't notified until after both happens)

Okay.

> Why would it break the contract? Note, I am referring to
> HttpRequest.cancel() not CompletableFuture.cancel()

Okay.
Still, there is no way to specify an async timeout (and the others I mentioned).

Consider this:

try
{
response = responseFuture.get(5, SECONDS);
}
catch (TimeoutException x)
{
// The response may have arrived after 5.1 s, but before we
execute the code below.

// Aborting a request that is instead successful !
boolean cancelled = request.cancel();
if (!cancelled)
{
// May be succeeded or failed, try again.
try
{
response = response.get();
}
catch (Exception xx)
{
// Yuck
}
}
}

Note that HttpRequest.cancel() does not return a boolean to know
whether the request has been really cancelled, or if it arrived in the
meanwhile.
I think it should (must).

Now compare to this:

try
{
response = client.newRequest(uri).timeout(5,
SECONDS).method("GET").response();
}
catch (TimeoutException x)
{
// Request already atomically aborted by the implementation.
}

Note that the latter solution will work also for async timeouts:

client.newRequest(uri).timeout(5,
SECONDS).method("GET").responseAsync().handle((r, x) ->
{
if (x instance of TimeoutException)
;// handle timeout
});

The API to specify timeouts is the same for both sync and async cases.

The idiom an application writer has to write when using
Future.get(time) is just cumbersome, I would advice against any usage
of Future methods in the API.
CompletableFuture is fine, as long as you don't use the Future methods.
If an application wants to use Future methods, they are on their own,
but at least they are not forced to.

-- 
Simone Bordet
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz

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

2015-09-28 Thread Michael McMahon 

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 always has this information because it's the one
that passed the ByteBuffer to the application, so it would be trivial
for the implementation to make this conversion.

Trivial for n = 1 perhaps. For n>1 the implementation must keep track
of the size of each ByteBuffer (the number of bytes contained in each)
passed to the application and as each buffer is acknowledged, the window
can be updated by the corresponding number of bytes. So, while the buffers
themselves are typically of fixed size, the amount of data contained 
might not be
as each probably corresponds to a packet on the network (which can vary 
in size obviously).

I think it's a little odd, but is doable.

Another question is what value would be application/response processor use
as the initial window size? Without knowing something about the likely 
buffer

size, it is hard to know what to use (except the value 1).


Perhaps, if n is required always to be equal to 1, then you could do it
because you would always be referring to the last buffer received
but it seems trickier if n > 1.

Consider onRequestBodyChunk(ByteBuffer) as Flow.Subscriber.onNext(ByteBuffer).
What an application wants to say here is "give me another buffer", not
"give me 42 bytes".

The 42 bytes are irrelevant because the implementation won't certainly
read 42 bytes *only* from the network: it typically has buffers of
certain size that it reuses for network reads.

The application will always be in control if it really needs 42 bytes,
whatever length is the buffer that the implementation passes to it.
It can buffer the bytes if they're less than 42, it can slice the
buffer if they're more than 42, it can do whatever it needs to do to
comply with the 42 bytes requirement.
What it needs from the implementation is just another ByteBuffer.


What I was saying was that completion of response also implies
competion of request (ie that it isn't notified until after both happens)

Okay.


Why would it break the contract? Note, I am referring to
HttpRequest.cancel() not CompletableFuture.cancel()

Okay.
Still, there is no way to specify an async timeout (and the others I mentioned).

Consider this:

try
{
 response = responseFuture.get(5, SECONDS);
}
catch (TimeoutException x)
{
 // The response may have arrived after 5.1 s, but before we
execute the code below.

 // Aborting a request that is instead successful !
 boolean cancelled = request.cancel();
 if (!cancelled)
 {
 // May be succeeded or failed, try again.
 try
 {
 response = response.get();
 }
 catch (Exception xx)
 {
 // Yuck
 }
 }
}

Note that HttpRequest.cancel() does not return a boolean to know
whether the request has been really cancelled, or if it arrived in the
meanwhile.
I think it should (must).

Now compare to this:

try
{
 response = client.newRequest(uri).timeout(5,
SECONDS).method("GET").response();
}
catch (TimeoutException x)
{
 // Request already atomically aborted by the implementation.
}

Note that the latter solution will work also for async timeouts:

client.newRequest(uri).timeout(5,
SECONDS).method("GET").responseAsync().handle((r, x) ->
{
 if (x instance of TimeoutException)
 ;// handle timeout
});

The API to specify timeouts is the same for both sync and async cases.

The idiom an application writer has to write when using
Future.get(time) is just cumbersome, I would advice against any usage
of Future methods in the API.
CompletableFuture is fine, as long as you don't use the Future methods.
If an application wants to use Future methods, they are on their own,
but at least they are not forced to.



We might reconsider this. Currently there is no timeout for blocking
calls, which I guess is problematic.

Thanks
Michael

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

2015-09-28 Thread Simone Bordet 
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 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 *block* via CF.get(time), which is of
> course against an async API and therefore not a viable solution.
>
> --
> Simone Bordet
> http://bordet.blogspot.com
> ---
> Finally, no matter how good the architecture and design are,
> to deliver bug-free software with optimal performance and reliability,
> the implementation technique must be flawless.   Victoria Livschitz



-- 
Simone Bordet
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz

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

2015-09-28 Thread Simone Bordet 
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 *block* via CF.get(time), which is of
course against an async API and therefore not a viable solution.

-- 
Simone Bordet
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz

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

2015-09-27 Thread Simone Bordet 
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 http://cr.openjdk.java.net/~michaelm/8087112/01/

It appears that none of my concerns expressed in this message:
http://mail.openjdk.java.net/pipermail/net-dev/2015-July/009048.html
have been addressed.
Am I looking at the right link ?

In particular:

A) lack of backpressure,
B) missing "request complete" event
C) missing timeouts (CompletableFuture.get(time) is obviously not the
right semantic)

are blockers for me.

The other points in my previous message are less blockers, but I would
still ask to consider them.

Thanks !

-- 
Simone Bordet
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz

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

2015-09-27 Thread Anthony Vanelverdinghe 

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 token, String errormsg) {
if (token.isEmpty() || token.chars().anyMatch(c -> !isTchar(c))) {
throw new IllegalArgumentException(errormsg);
}
}

private static boolean isTchar(int c) {
return c >= 0x30 && c <= 0x39 // 0 - 9
|| (c >= 0x61 && c <= 0x7a) // a - z
|| (c >= 0x41 && c <= 0x5a) // A - Z
|| (c >= 0x21 && c <= 0x2e && c != 0x22 && c != 0x28 && c 
!= 0x29 && c != 0x2c)

|| (c >= 0x5e && c <= 0x60)
|| (c == 0x7c) || (c == 0x7e);
}

PS: I have signed the OCA [2], so you can reuse any code if you wish

[1] 
http://cr.openjdk.java.net/~michaelm/8087112/01/src/java.base/share/classes/sun/net/httpclient/Utils.java.html

[2] http://www.oracle.com/technetwork/community/oca-486395.html#v

Kind regards,
Anthony


On 25/09/2015 21:59, Anthony Vanelverdinghe wrote:

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 
http://cr.openjdk.java.net/~michaelm/8087112/01/src/java.base/share/classes/sun/net/httpclient/SSLTunnelConnection.java.html

at line 56, replace
return connected & delegate.connected();
with
return connected && delegate.connected();

in 
http://cr.openjdk.java.net/~michaelm/8087112/01/src/java.base/share/classes/java/net/package-info.java.udiff.html

replace "superceded" with "superseded"

On a final note, when will the new API be integrated in the weekly 
early access builds [2]?


[1] https://tools.ietf.org/html/rfc7230#section-3.2.6
[2] https://jdk9.java.net/download/

Kind regards
Anthony


On 28/08/2015 22:50, 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 http://cr.openjdk.java.net/~michaelm/8087112/01/

Below is a description of the API classes and the relationship with
their implementation classes.

Thanks
Michael


---
java.net
---
HttpClient
||
\/
Client
||
\/
sun.net.httpclient.HtpClientImpl


HttpRequest
||
\/
Request
||
\/
sun.net.httpclient.HtpRequestImpl


--
sun.net.httpclient
--
Implementation classes showing calling relationship. Classes
below have synchronous and asynchronous variants for sending/receiving

HttpRequestImpl - overall implementation of HTTP
  ||
  \/
MultiExchange - manages multiple request/response exchanges that
  ||occur as a result of errors or filters (only used 
internally)

  \/
Exchange - manages one request response interaction, possibly including
  ||   intermediate 100 Continue response.
  ||
  \/
ExchangeImpl - separates request sending, response receiving. Class is
  ||   abstract with different implementations for Http/1 and 
Http/2

  \/
Http1Exchange - implementation for HTTP/1. Manages TCP connection pools
 |||HttpConnection (and its subtypes). Interacts with NIO 
selector

 |||in HttpClientImpl
 ||+---+
 ||   ||
 \/   \/
Http1Request   Http1Response


HttpConnection
 | (subtypes)
 |
PlainHttpConnection
PlainProxyConnection
PlainTunnelingConnection
SSLConnection
SSLTunnelingConnection

FilterFactory - factory for filters
HeaderFilter - internal API for filters (not exposed externally)
AuthenticationFilter - implements HTTP Basic auth
RedirectionFilter - implements automatic redirection of 3XX responses
CookieFilter - implements CookieManager interface

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

2015-09-25 Thread Anthony Vanelverdinghe 

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 
http://cr.openjdk.java.net/~michaelm/8087112/01/src/java.base/share/classes/sun/net/httpclient/SSLTunnelConnection.java.html

at line 56, replace
return connected & delegate.connected();
with
return connected && delegate.connected();

in 
http://cr.openjdk.java.net/~michaelm/8087112/01/src/java.base/share/classes/java/net/package-info.java.udiff.html

replace "superceded" with "superseded"

On a final note, when will the new API be integrated in the weekly early 
access builds [2]?


[1] https://tools.ietf.org/html/rfc7230#section-3.2.6
[2] https://jdk9.java.net/download/

Kind regards
Anthony


On 28/08/2015 22:50, 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 http://cr.openjdk.java.net/~michaelm/8087112/01/

Below is a description of the API classes and the relationship with
their implementation classes.

Thanks
Michael


---
java.net
---
HttpClient
||
\/
Client
||
\/
sun.net.httpclient.HtpClientImpl


HttpRequest
||
\/
Request
||
\/
sun.net.httpclient.HtpRequestImpl


--
sun.net.httpclient
--
Implementation classes showing calling relationship. Classes
below have synchronous and asynchronous variants for sending/receiving

HttpRequestImpl - overall implementation of HTTP
  ||
  \/
MultiExchange - manages multiple request/response exchanges that
  ||occur as a result of errors or filters (only used 
internally)

  \/
Exchange - manages one request response interaction, possibly including
  ||   intermediate 100 Continue response.
  ||
  \/
ExchangeImpl - separates request sending, response receiving. Class is
  ||   abstract with different implementations for Http/1 and 
Http/2

  \/
Http1Exchange - implementation for HTTP/1. Manages TCP connection pools
 |||HttpConnection (and its subtypes). Interacts with NIO 
selector

 |||in HttpClientImpl
 ||+---+
 ||   ||
 \/   \/
Http1Request   Http1Response


HttpConnection
 | (subtypes)
 |
PlainHttpConnection
PlainProxyConnection
PlainTunnelingConnection
SSLConnection
SSLTunnelingConnection

FilterFactory - factory for filters
HeaderFilter - internal API for filters (not exposed externally)
AuthenticationFilter - implements HTTP Basic auth
RedirectionFilter - implements automatic redirection of 3XX responses
CookieFilter - implements CookieManager interface

RFR: 8087112: HTTP API and HTTP/1.1 implementation

2015-08-28 Thread Michael McMahon 

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 classes.

Thanks
Michael


---
java.net
---
HttpClient
||
\/
Client
||
\/
sun.net.httpclient.HtpClientImpl


HttpRequest
||
\/
Request
||
\/
sun.net.httpclient.HtpRequestImpl


--
sun.net.httpclient
--
Implementation classes showing calling relationship. Classes
below have synchronous and asynchronous variants for sending/receiving

HttpRequestImpl - overall implementation of HTTP
  ||
  \/
MultiExchange - manages multiple request/response exchanges that
  ||occur as a result of errors or filters (only used 
internally)

  \/
Exchange - manages one request response interaction, possibly including
  ||   intermediate 100 Continue response.
  ||
  \/
ExchangeImpl - separates request sending, response receiving. Class is
  ||   abstract with different implementations for Http/1 and 
Http/2

  \/
Http1Exchange - implementation for HTTP/1. Manages TCP connection pools
 |||HttpConnection (and its subtypes). Interacts with NIO 
selector

 |||in HttpClientImpl
 ||+---+
 ||   ||
 \/   \/
Http1Request   Http1Response


HttpConnection
 | (subtypes)
 |
PlainHttpConnection
PlainProxyConnection
PlainTunnelingConnection
SSLConnection
SSLTunnelingConnection

FilterFactory - factory for filters
HeaderFilter - internal API for filters (not exposed externally)
AuthenticationFilter - implements HTTP Basic auth
RedirectionFilter - implements automatic redirection of 3XX responses
CookieFilter - implements CookieManager interface