Re: RFR: 8273655: content-types.properties files are missing some common types

[Anthony Vanelverdinghe]

On Tue, 14 Sep 2021 09:46:21 GMT, Julia Boes  wrote:

> This change adds some common types to the content-type.properties files, 
> notably .js, .css, and .jar, as well as some others. 
> 
> The duplicated entry for .zip is removed from the Windows properties file.

src/java.base/unix/classes/sun/net/www/content-types.properties line 266:

> 264: 
> 265: text/javascript: \
> 266: description=Javascript File;\

JavaScript

src/java.base/unix/classes/sun/net/www/content-types.properties line 323:

> 321:  file_extensions=.xml
> 322: 
> 323: application/rtf: \

I'd also add an entry for `text/markdown`

src/java.base/unix/classes/sun/net/www/content-types.properties line 324:

> 322: 
> 323: application/rtf: \
> 324:  description=Wordpad Document;\

WordPad

src/java.base/unix/classes/sun/net/www/content-types.properties line 326:

> 324:  description=Wordpad Document;\
> 325:  file_extensions=.rtf;
> 326: 

Maybe add entries for the equivalent OpenDocument formats as well?

src/java.base/unix/classes/sun/net/www/content-types.properties line 327:

> 325:  file_extensions=.rtf;
> 326: 
> 327: application/xls: \

application/vnd.ms-excel

src/java.base/unix/classes/sun/net/www/content-types.properties line 328:

> 326: 
> 327: application/xls: \
> 328:  description=XLS File;\

I'd describe the Office formats as "Microsoft Word/Excel/PowerPoint"

src/java.base/unix/classes/sun/net/www/content-types.properties line 331:

> 329:  file_extensions=.xls;
> 330: 
> 331: application/xlsx: \

application/vnd.openxmlformats-officedocument.spreadsheetml.sheet

src/java.base/unix/classes/sun/net/www/content-types.properties line 339:

> 337:  file_extensions=.csv;
> 338: 
> 339: application/pptx: \

application/vnd.openxmlformats-officedocument.presentationml.presentation

src/java.base/unix/classes/sun/net/www/content-types.properties line 343:

> 341:  file_extensions=.pptx;
> 342: 
> 343: application/ppt: \

application/vnd.ms-powerpoint

src/java.base/unix/classes/sun/net/www/content-types.properties line 347:

> 345:  file_extensions=.ppt;
> 346: 
> 347: application/7z: \

application/x-7z-compressed

src/java.base/unix/classes/sun/net/www/content-types.properties line 348:

> 346: 
> 347: application/7z: \
> 348:  description=7z File;\

7-Zip File

src/java.base/unix/classes/sun/net/www/content-types.properties line 351:

> 349:  file_extensions=.7z;
> 350: 
> 351: application/doc: \

application/msword

src/java.base/unix/classes/sun/net/www/content-types.properties line 355:

> 353:  file_extensions=.doc;
> 354: 
> 355: application/docx: \

application/vnd.openxmlformats-officedocument.wordprocessingml.document

src/java.base/unix/classes/sun/net/www/content-types.properties line 359:

> 357:  file_extensions=.docx;
> 358: 
> 359: application/php: \

Since e.g. `.java` and `.pl` are already listed as file extensions under 
`text/plain`, I'd add `.php` there, and add `.adoc` (AsciiDoc), `.ts` 
(TypeScript) and `.py` (Python) as well.

src/java.base/unix/classes/sun/net/www/content-types.properties line 363:

> 361:  file_extensions=.php;
> 362: 
> 363: application/rar: \

application/vnd.rar

src/java.base/windows/classes/sun/net/www/content-types.properties line 30:

> 28: application/octet-stream: \
> 29:   description=Generic Binary Stream;\
> 30:   file_extensions=.saveme,.dump,.hqx,.arc,.obj,.lib,.bin,.exe,.gz

`.gz` has a registered media type and should be a separate entry

-

PR: https://git.openjdk.java.net/jdk/pull/5506

Re: RFR: 8235459: HttpRequest.BodyPublishers#ofFile(Path) assumes the default file system

[Anthony Vanelverdinghe]

Hi Julia

On 06/03/2020 14:04, Julia Boes wrote:

Hi Anthony,

Thanks for your comments.

For distinguishing the non-default filesystem case, an alternative to 
using the try-catch block

is an if-else block with the same check as is done in Path::toFile:
    if (path.getFileSystem() == FileSystems.getDefault())


Path::toFile can be overridden so the try-catch block potentially 
covers more cases.

Good point, I hadn't thought of that.




When setting `length`, the catch clause can be limited to 
IOException, rather than Exception.

Good catch, changed.


Maybe change the method `RuntimeException toUncheckedException(...)` 
to `void throwAsUncheckedException(...)`
and then instead of `throw toUncheckedException(...)` use 
`throwAsUncheckedException(...)`


In that case FilePublisher::createInputStream would be missing a 
return statement. The original version makes it explicit that we 
always throw something - I'll stick to the original version if that's 
ok with you.

I see now, thanks, and agree the original version is preferable.

Webrev is updated.

Cheers,

Julia



Kind regards,
Anthony

Re: RFR: 8235459: HttpRequest.BodyPublishers#ofFile(Path) assumes the default file system

[Anthony Vanelverdinghe]

Hi Julia

Here's some suggestions w.r.t. RequestPublishers.java:

For distinguishing the non-default filesystem case, an alternative to 
using the try-catch block

is an if-else block with the same check as is done in Path::toFile:
    if (path.getFileSystem() == FileSystems.getDefault())

When setting `length`, the catch clause can be limited to IOException, 
rather than Exception.


Maybe change the method `RuntimeException toUncheckedException(...)` to 
`void throwAsUncheckedException(...)`
and then instead of `throw toUncheckedException(...)` use 
`throwAsUncheckedException(...)`


Kind regards,
Anthony

On 05/03/2020 14:50, Julia Boes wrote:

Hi,

Please see this fix that adds support for non-default file systems to 
the HttpClient. More specifically, the change is in 
RequestPublishers.FilePublisher where an UnsupportedOperationException 
is thrown if a java.io.File cannot be obtained. The exception is now 
caught and a function is used to obtain an InputStream with a 
privileged scope based on the captured AccessControlContext.


Bug: https://bugs.openjdk.java.net/browse/JDK-8235459

CSR: https://bugs.openjdk.java.net/browse/JDK-8240526

Webrev: http://cr.openjdk.java.net/~jboes/webrevs/8235459/webrev.01/

Tests were run with repeat 100 cross-platform. A similar change to the 
receiving side is currently work in progress.


Regards,

Julia

Re: HTTP client API

[Anthony Vanelverdinghe]

Hi Tobias

As far as I know, the latest published Javadoc is at [1] (which was 
linked to in [2]). However, this Javadoc doesn't exactly match the 
current code either.


Below are some quick replies to some of your feedback.

1.)
> What is the advantage of the Publisher / 
Subscriber API over plain old InputStream / OutputStream 
based APIs?
InputStream / OutputStream are blocking APIs, whereas Publisher / 
Subscriber are part of the Flow API, which corresponds to the Reactive 
Streams specification [3].


> there is an implementation that discards all data [...] (and its 
generic type is called U rather than T)
The generic type is called U to avoid shadowing the type parameter in 
the enclosing HttpResponse.BodyProcessor


> one thing I found particularly challenging was the control flow 
progression [...] because it is push based and forces an application to 
relinquish control to the library rather than pulling data out of the 
library.
The application is still in control by means of its Flow.Subscription 
(see Flow.Subscriber::onSubscribe), isn't it?


> asByteArrayConsumer(Consumer> consumer): Why is this 
an Optional? What logic decides whether an empty response body will be 
represented as a present byte[0] or an absent value?
As the Javadoc says, it's an Optional to be able to mark the end of the 
body. In case of an empty response body, the Consumer will therefore 
receive an Optional.empty() value.


2.) what happens when name is null, is specified by the following note 
in the package Javadoc: "Unless otherwise stated, null parameter values 
will cause methods of all classes in this package to throw 
NullPointerException."


3.) allowing custom redirection policies was brought up before [4][5], 
proposing that a redirection policy would simply be a 
BiFunction>. Besides, 
analog to e.g. java.nio.file.StandardOpenOption, there would also be a 
StandardRedirectionPolicy enum [6].


4.) it doesn't, as is specified in HttpClient.Builder::version

5.) this was brought up in [7], and was addressed, I believe, in the 
latest API [1] by replacing CookieManager with java.net.CookieHandler. 
However, this change isn't in the current code (yet?).


6.) I'd assume the Executor is used to handle asynchronous requests 
(HttpClient::sendAsync in [1]). So by using a fixed thread pool, one can 
control the maximum number of concurrent requests.


Hope this helps,
Anthony

[1] http://cr.openjdk.java.net/~michaelm/httpclient/api.1/
[2] 
http://mail.openjdk.java.net/pipermail/net-dev/2016-September/010238.html

[3] http://www.reactive-streams.org/
[4] http://mail.openjdk.java.net/pipermail/net-dev/2016-February/009547.html
[5] http://mail.openjdk.java.net/pipermail/net-dev/2016-March/009608.html
[6] 
http://jep110-anthonyvdotbe.rhcloud.com/api/jep110/StandardRedirectionPolicy.html

[7] http://mail.openjdk.java.net/pipermail/net-dev/2016-August/010187.html


On 24/10/2016 21:33, Tobias Thierer wrote:


Hi Michael and others -


Thanks for publishing the latest HTTP client API docs 
(already 
slightly outdated again), as well as for publishing the current draft 
code in the sandbox repository!



Below is some concrete feedback, questions and brainstorming on how to

(a) increase the usefulness or

(b) decrease the semantic weight

of the API. Note that most of this is driven only by inspection of the 
API and some brief exploration of the implementation code, not (yet) 
by a substantial effort to write proof of concept client applications. 
I’d love if I could help make this API as useful to applications as 
possible, so I’d appreciate your feedback on how I can best do that 
and what the principles were that guided your design choices.



1.) The HttpRequest.BodyProcessor 
and 
HttpResponse.BodyProcessor 
abstractions 
seem particularly hard to grasp / have a high semantics weight.


 *

What purpose does the abstraction of a BodyProcessoraim to fulfill
beyond what the (simpler) abstraction of a Bodycould be?

 o

Instead of describing the abstraction as a “processor” of
ByteBuffers / Java objects, wouldn’t it be simpler to say to
say that request / response bodiesare ByteBuffer / Java object
sources/ sinks? What is the advantage of the
Publisher / Subscriber API over plain
old InputStream / OutputStream based APIs?

 o

The term “processor” and the description of “converting
incoming buffers of data to some user-defined object type T”
is especially confusing (increases the semantic weight of the
abstraction) given that there is an implementation that
discards all data

Re: HTTP client API

[Anthony Vanelverdinghe]

Hi Michael

What's the rationale for not turning all public classes into interfaces, 
since none of them contain any actual implementation code?


On another note, I fail to see the point of 
HttpClient.Builder::priority: as far as I understand, HTTP/2 priority 
only comes into play when multiple resources depend on the same 
resource. But since there's no way to express dependencies between 
resources, how would this be useful?


Lastly, some minor remarks: I believe the following factory methods 
should be moved for consistency

- HttpRequest::noBody --> HttpRequest.BodyProcessor::noBody
- HttpResponse::multiFile --> HttpResponse.MultiProcessor::asFiles

and that "? extends T" should be used instead of "T", in the return type 
of the following methods

- HttpResponse.BodyHandler::apply, i.e. BodyProcessor
- HttpResponse.BodyProcessor::getBody, i.e. CompletionStage
- HttpResponse.MultiProcessor::onRequest, i.e. Optionalextends T>>


Kind regards,
Anthony


On 7/09/2016 19:15, Michael McMahon wrote:

Hi Wenbo,

First, sorry for the delay in replying. We took your comments and 
prototyped how the major ones

might be accommodated. In particular, we did the following:

- moved "business logic" out of HttpRequest. The methods for sending 
requests now
   exist in HttpClient. Given that requests are no longer associated 
with a client and can
   be sent through any client, this means that some properties of a 
request that used
   to be inherited from the client can no longer be visible in the 
HttpRequest object,
   but that is not a problem as far as I can see. This changes the 
look of the sample code

   in HttpRequest, but doesn't make it any harder to read.

- changed some method names as suggested eg newBuilder()

- added protected constructors to the public classes. This allows 
alternative implementations

   for use in mocking/test frameworks etc.

- changed the PUT, POST, GET methods in the request builder to only 
set the method type.
   PUT, POST takes the request body processor as parameter. A new 
build() method returns

   the HttpRequest.

- added a method to HttpResponse which returns the "final" request 
that was sent on the wire
   for the particular response, which might be different from the user 
initiated request.


I put an updated apidoc [1] and specdiff [2] up to show the changes. 
In particular, the sample

code described in the HttpClient docs is updated to reflect the changes.

Thanks,
Michael

[1] http://cr.openjdk.java.net/~michaelm/httpclient/api.1/

[2] 
http://cr.openjdk.java.net/~michaelm/httpclient/specdiffout.1/package-summary.html


On 26/08/2016, 07:59, Wenbo Zhu wrote:

Hi Michael,

Thanks for the update! The adoption of the Flow API is indeed a big 
improvement (async flow-control is very hard to get right).


Attached is a feedback doc on this new version. One specific idea to 
discuss is whether it's possible to release the new HTTP client API 
as a standalone library (that works on JDK 9).


Thanks,
Wenbo



On Mon, Aug 22, 2016 at 12:56 AM, Michael McMahon 
mailto:michael.x.mcma...@oracle.com>> 
wrote:


There is an updated version of the HTTP client API doc [1] and a
specdiff [2] showing the changes
proposed from the current version in JDK9 dev.

The main changes are:

1) The request and response processors are now based on
Flow.Publisher and Flow.Subscriber

2) Response bodies are retrieved synchronously with the response
headers as part of the
HttpRequest.response() and responseAsync() methods

3) Because of the change above, to allow code to examine the
headers and decide what to do
with the body before retrieving it, there is a new entity
called a HttpResponse.BodyHandler
which is given the status code and headers, and which returns
a HttpResponse.BodyProcessor.
Static implementations of both body handlers and body
processors are provided, to make the
simple cases easy to code.

We are currently working in the sandbox repository again and will
have these changes
in the main JDK9 dev forest soon.

Thanks,

Michael

[1] http://cr.openjdk.java.net/~michaelm/httpclient/api/


[2]

http://cr.openjdk.java.net/~michaelm/httpclient/specdiffout/package-summary.html

JEP 110: leeway for API changes?

[Anthony Vanelverdinghe]

Hi

With the JDK's "Feature Complete" milestone coming up next week: how 
much leeway is there still for API changes? I've made API suggestions 
(a.o. in [1]), ranging from changing 
HttpResponse.multiProcessor::onStart's return type (from BiFunction<..., 
Boolean> to BiPredicate), to building on the j.u.c.Flow API & 
eliminating HttpResponse.MultiProcessor. So I'm wondering what's still 
on on the table for JEP 110 w.r.t. API changes?


[1] http://mail.openjdk.java.net/pipermail/net-dev/2016-March/009608.html

Kind regards,
Anthony

Re: RFR JDK-8087113: Websocket API and implementation

[Anthony Vanelverdinghe]

Hi Pavel

Thanks for your considerate response, replies are inline.

On 4/04/2016 13:08, Pavel Rappo wrote:

Hi Anthony, thanks a lot for looking into this!


On 3 Apr 2016, at 17:45, Anthony Vanelverdinghe 
 wrote:

Here are my suggestions concerning the public types:

java.net.http.WebSocket
- order the arguments of
static Builder newBuilder(URI uri, HttpClient client, Listener listener)
consistently with the 2-argument method:
static Builder newBuilder(URI uri, Listener listener, HttpClient client)

I see your concern. I've considered  using the same signature. But I chose the
other one for a reason. There are pros and cons for both versions, of course.
If we choose the proper telescoping version

newBuilder(URI uri, Listener listener, HttpClient client)

then it would be more consistent with the 2-arg method indeed. On the other hand
it would make a use of an in-place created Listener a bit more ugly. Imagine a
huge anonymous "listener" in the middle with an almost unnoticeable tiny
"client" in the end of the argument list:

 WebSocket.newBuilder(URI.create("ws://myws"), new Listener() {

 @Override
 public void onOpen(WebSocket webSocket) {
 ...
 }

 ...

 @Override
 public CompletionStage onPong(WebSocket webSocket, ByteBuffer 
message) {
 ...
 }
 }, client);

I would prefer to have something extensible moved to the very end of the list of
arguments. But that's just my opinion.

Good point, in that case the current ordering is indeed clearer.

- remove CompletableFuture sendText(Stream 
message):
there are many candidate convenience methods, and the choice for Stream seems arbitrary. For example, why this one, and not methods with char[] 
(analog to the byte[] method for sendBinary), Iterable (as 
used by java.nio.file.Files::write), Reader, ...

java.util.stream.Stream would be a more general source than java.lang.Iterable.


convenience methods can always be added in a later version, based on empirical 
evidence of which convenience methods would add most value

Absolutely!


- remove CompletableFuture sendBinary(byte[] message, boolean isLast):
same motivation as the previous one

- add CompletableFuture sendBinary(ByteBuffer message)
to be consistent with sendText, which has the single-parameter method 
sendText(CharSequence message)

I will think about it.


- CompletableFuture sendClose(CloseCode code, CharSequence reason)
change the type of "reason" to String

What's the rationale behind this?
Unlike with the sendText methods, I don't see the added value here. For 
example, compare this with the method "header(String name, String 
value)" in the Builder interface. To me this is the same: it might be 
useful for callers if the parameters of that method were declared as a 
CharSequence, but they aren't, and it would not be "Java-like" 
(according to my totally subjective definition of that term) for them to 
be either. Anyway, just my 2 cents.

java.net.http.WebSocket.Builder
- Builder connectTimeout(long timeout, TimeUnit unit)
use a single java.time.Duration argument instead

I'm not too familiar with the new java.time API, but maybe you could explain
what would some advantages be over the java.util.concurrent one?

The one I can think of immediately is the encapsulation and consistency check
would be done by java.time rather than by java.net.http.WebSocket. On the other
hand java.util.concurrent.TimeUnit is more familiar.
I'd argue that, since java.net.http will be a new package (& separate 
module), its API should make as good use as possible of the available 
types in the JDK. And in that regard, I believe a single Duration 
parameter is clearer. And, though it doesn't apply in this case: an 
advantage of Duration is that you can cleanly return it: Duration 
getTimeout(), in which case it would be natural to have the setter 
declared as void setTimeout(Duration t). While j.u.c.TimeUnit is indeed 
more familiar, I don't see why this would speak in its advantage: it's 
just a natural consequence from the fact that it's been around 10 years 
longer than Duration already.

java.net.http.WebSocket.Listener
- onText/onBinary/onPing/onPong
return an immediately-complete CompletionStage (i.e. 
CompletableFuture.completedStage(null)) instead of null

We've been through this. It's a matter of both performance AND convenience. A
user gets both convenience of returning null and reduction of allocation.
Moreover if we go with disallowing null, people might be tempted to use a single
shared instance

CompletableFuture.completedStage(null)

as a return value. I'd better stay away from it (though it seems technically
possible, it still feels hacky).
Sorry, I hadn't gone through previous discussions. I assume the option 
of returning Optional> has already been considered as 
wel

Re: RFR JDK-8087113: Websocket API and implementation

[Anthony Vanelverdinghe]

Hi Pavel

Here are my suggestions concerning the public types:

java.net.http.WebSocket
- order the arguments of
static Builder newBuilder(URI uri, HttpClient client, Listener listener)
consistently with the 2-argument method:
static Builder newBuilder(URI uri, Listener listener, HttpClient client)

- remove CompletableFuture sendText(Stream 
message):
there are many candidate convenience methods, and the choice for 
Stream seems arbitrary. For example, why this 
one, and not methods with char[] (analog to the byte[] method for 
sendBinary), Iterable (as used by 
java.nio.file.Files::write), Reader, ...
convenience methods can always be added in a later version, based on 
empirical evidence of which convenience methods would add most value


- remove CompletableFuture sendBinary(byte[] message, boolean isLast):
same motivation as the previous one

- add CompletableFuture sendBinary(ByteBuffer message)
to be consistent with sendText, which has the single-parameter method 
sendText(CharSequence message)


- CompletableFuture sendClose(CloseCode code, CharSequence reason)
change the type of "reason" to String

java.net.http.WebSocket.Builder
- Builder connectTimeout(long timeout, TimeUnit unit)
use a single java.time.Duration argument instead

java.net.http.WebSocket.Listener
- onText/onBinary/onPing/onPong
return an immediately-complete CompletionStage (i.e. 
CompletableFuture.completedStage(null)) instead of null

return CompletionStage instead of CompletionStage

- onText
change the type of the "message" parameter with ByteBuffer, and specify 
it contains UTF-8 encoded bytes & has a backing array


java.net.http.WebSocket.Text
remove this interface. I believe the Text interface doesn't carry its 
weight. By specifying the Listener::onText method as above, one can 
easily do something like new String().getBytes(message.array(), UTF_8) 
to get a CharSequence


java.net.http.WebSocketHandshakeException
- extend IOException, consistently with all other exception classes in 
java.net and javax.net
- if HttpResponse can indeed be null (as is documented in getResponse), 
the constructor's current implementation will throw a 
NullPointerException in this case


Kind regards,
Anthony

On 25/03/2016 17:21, Pavel Rappo wrote:

Hi,

Could you please review my change for JDK-8087113

http://cr.openjdk.java.net/~prappo/8087113/webrev.03/

This webrev contains initial implementation and basic tests for WebSocket API.
Specification conformance & interoperability testing has been performed with
Autobahn Testsuite [1]. As for API tests, I will provide more of them later.

Thanks,
-Pavel


[1] http://autobahn.ws/testsuite/

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

[Anthony Vanelverdinghe]

d at least 
provide all redirection URIs, not just the last one. Ideally, it should 
give access to all intermediate requests/responses.


=== HttpResponse
- multiFile: in my opinion, its usefulness is limited because:
  - it doesn't make a distinction between main response and push 
promises. In case a redirection occurred, it's even impossible to know 
which URI corresponds to the main response.
  - the resultant Map is only available once all responses are 
received. This is problematic, for example, if the client requests a 
"video playlist file", and the server pushes all videos in the playlist. 
In this case, the client should be able to process the playlist (i.e. 
main response must be distinguishable from push promises) and start 
playing the first video without having to wait for all videos to be 
downloaded
  A possible way to address these points, would be to return a 
MultiResponse, instead of the current Map:

  interface MultiResponse {
T mainResponse();
ConcurrentMap pushPromises();
boolean isDone();
  }
  the MultiProcessor could then add push promise responses as they 
become available, and "isDone" returns whether all push promises have 
been received.
  For this, I introduced a "HttpResponse send(HttpRequest r, 
BodyHandler bh, PushResourceHandler prh)" method on HttpClient 
(please refer to the Javadoc [6] for the definition of PushResourceHandler)


=== HttpResponse.MultiProcessor
- onStart: this must return BiPredicate<...> instead of BiFunction<..., 
Boolean>


=== RedirectFilter
- DEFAULT_MAX_REDIRECTS = 5: there should be no limit on the number of 
redirections, other than the detection of cycles [7]
- it must only handle the specific codes it can reasonably handle, not 
the whole range [300-399]
  for example, it currently throws an UncheckedIOException if no 
Location header is present, but response code 300 might not have, and 
304 does not have, such a header

- it should only automatically redirect requests with safe methods
- it should correctly rewrite methods where appropriate (e.g. changing 
POST into GET, cf. [8])
I introduced a BiFunction, Integer, 
Optional> for this:
- the library could provide some simple standard redirection policies, 
which only redirects GET requests with status codes of 301/302/303/307 e.g.
- application developers could write custom redirection policies, 
creating their own HttpRequest from the redirection response, imposing a 
limit on the number of redirects (using the Integer parameter of the 
BiFunction), etc.


Any and all feedback is welcome.

Kind regards,
Anthony

[1] http://mail.openjdk.java.net/pipermail/net-dev/2016-February/009547.html
[2] http://tools.ietf.org/html/rfc7231#section-7.1.1.2
[3] http://tools.ietf.org/html/rfc7230#section-4.3
[4] http://tools.ietf.org/html/rfc7230#section-6.4
[5] https://tools.ietf.org/html/rfc6454
[6] http://jep110-anthonyvdotbe.rhcloud.com/api/
[7] http://tools.ietf.org/html/rfc7231#section-6.4
[8] https://en.wikipedia.org/wiki/Post/Redirect/Get


On 22/02/2016 17:08, Michael McMahon wrote:

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

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

[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 where possible
- sslParameters(): change to "SSLParameters sslPar

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

[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

[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

Re: HTTP 2 client API

[Anthony Vanelverdinghe]

Hi Michael

Further feedback is in-line (please note that, to improve readability, I 
took the liberty to discard points without further feedback).


On 25/03/2015 18:33, Michael McMahon wrote:
* how is compression (the 
Accept-Encoding/Content-Encoding/Transfer-Encoding headers) handled? 
Is this handled transparently by the HttpClient? For example, if I 
request a text document, will the client automatically send a header 
"Accept-Encoding: gzip, deflate" (this is the current default in 
Firefox)? And once I receive the response, transparently decompress 
it, so I can just do "response.body(asString())"?
It wouldn't be transparent as such. But, it should be possible to 
implement it using HttpRequestBodyProcessor and 
HttpResponseBodyProcessor, but I wasn't
planning to do it as part of this work initially. Someone could define 
a class like: [...]
After reading up on the different headers, I agree that 
Accept-Encoding/Content-Encoding must not be handled by the API. 
However, I believe that the TE/Transfer-Encoding headers should be fully 
supported, by  adding a "compression(boolean enable)" method to 
HttpClient.Builder and HttpRequest.Builder. This method would allow 
transparent compression by supporting at least one of the standardized 
algorithms (preferably gzip) [ 
http://www.iana.org/assignments/http-parameters/http-parameters.xhtml#transfer-coding 
].


Basically, I believe that the API should offer full support for the 
"low-level" RFC 7230 (Message Syntax and Routing), in which the 
"Transfer Codings" section specifies these headers. And since 
TE/Transfer-Encoding is the correct way for transparent on-the-fly 
compression, it seems logical to include support for it in the API (to 
me, it's on the same level as pipelining support, which is included in 
the API already).


* HttpHeaders: for the proposed first*() methods, please also 
consider the signature: Optional firstValueAsT(String name). This 
way, the application developer can decide whether to use a default 
value or to throw an exception (or anything else) when the header is 
absent.

How would that be implemented without knowing the type  ?
Sorry for the confusion, this wasn't meant to represent a generic 
method. I just meant to say: please consider returning an Optional 
instead of the current behavior of returning null or taking a "default 
value" parameter. So instead of the current methods:

String firstValue(String name)
long firstValueAsLong(String name, long defaultValue)

consider these methods instead:
Optional firstValue(String name)
OptionalLong firstValueAsLong(String name)

The advantage of the latter methods would be that you can say for example:
headers.firstValue("Content-Type").orElse("application/octet-stream")
headers.firstValueAsLong("Content-Length").orElseThrow(() -> new 
IllegalStateException())


* HttpRequest::fromString and HttpResponse::asString should take a 
java.nio.charset.Charset as parameter

instead of the String name of the Charset? Will consider.
I'd just like to point out that my main motivation for this is 
java.nio.charset.StandardCharsets. So for charsets such as UTF-8, UTF-16 
and US-ASCII, one can use a static import & simply write e.g. 
asString(UTF_8). Of course, if it's estimated that most of the uses of 
this method will use a non-standard charset, then it may be undesirable 
having to write asString(Charset.forName("Windows-1252")) instead.


On a sidenote, the method asString(String charset) in HttpResponse 
should specify to throw an UnsupportedCharsetException. And in the 
method fromString(String s, String charset) in HttpRequest, the 
parameter "s" should be renamed to "body".


* HttpResponse: in my opinion, "asFile" should take a Path as 
parameter & the parameter name should be "file", instead of "filename"

same as above
An advantage of taking a Path, would be that you don't have to embed 
(OS-specific) file separators to construct the desired argument:

asFile(downloadDirectory.resolve(Paths.get("documents", "foo.pdf"));

* please consider adding a HttpResponseBodyProcessor implementation 
"asDefined()", which uses the same mechanism as 
java.net.URLConnection::getContent (i.e. using the Content-Type 
header & ContentHandlers) to determine the object to return. This 
would allow for easy migration from the old API to the new API. (the 
"defined" in the method name refers to the fact that the value of the 
Content-Type header is used)
How would you determine the type of object to return? I believe that 
was one limitation of that API in URLConnection
This would be a HttpResponseBodyProcessor, and the type of 
object would just be whatever type the chosen ContentHandler returns 
(where the ContentHandler is chosen according to the algorithm specfied 
in java.net.ContentHandler).


After giving this some more thought, I think the proposed 
HttpResponseBodyProcessor isn't what I'm after. Actually, what I like 
about the old API, is that I can write ContentHandlers, specify the 

Re: HTTP 2 client API

[Anthony Vanelverdinghe]

Hi Michael

Please find my feedback on the current API docs below. It's divided in 3 
parts: API (mostly questions), documentation (mostly suggestions for 
clarifications) and spelling.

Thanks in advance for your consideration.

Kind regards, Anthony

=== API ===
* will there be support for HTTP 2.0 stream prioritization? For example 
by providing a method requestHttp2(int priority) in 
HttpClient.Builder/HttpRequest.Builder? This way one could create 2 
clients with a different priority: a low-priority one for background 
downloads of documents/large files, and a high-priority one for status 
updates. Then the 2 clients would use the same TCP connection, but 
different streams with different priorities.


* which convenience methods will be added to existing classes, if any? 
I'm particularly thinking of URL, which already has 
openConnection/openStream convenience methods for the old API.


* how is compression (the 
Accept-Encoding/Content-Encoding/Transfer-Encoding headers) handled? Is 
this handled transparently by the HttpClient? For example, if I request 
a text document, will the client automatically send a header 
"Accept-Encoding: gzip, deflate" (this is the current default in 
Firefox)? And once I receive the response, transparently decompress it, 
so I can just do "response.body(asString())"?


* how is caching handled? Is there anything analog to "useCaches" in 
java.net.URLConnection?


* HttpClient.Builder: concerning "followRedirects", I feel there are 2 
more options that ought to be possible to be set: "same-protocol" (i.e. 
the current java.net.HttpURLConnection behavior) and "secure" (all 
redirects are allowed, except for redirects from https to http). A 
possible solution might be to introduce a "RedirectPolicy" enum with 4 
constants: NEVER, ALWAYS, SAME_PROTOCOL, SECURE. Then the method would 
become "followRedirects(RedirectPolicy policy)".


* HttpClient.Builder: the documentation for "requestHttp2" says: "If 
that fails, then following requests will not attempt to upgrade again." 
How is it determined that the upgrade to HTTP/2 fails? For example, is 
the client able to distinguish "the upgrade to HTTP/2 failed" from "some 
error occured which caused the request to fail" (e.g. the server is 
down)? And how can I force to attempt to upgrade again, for a specific 
origin and/or all origins? For example in the case of application 
servers, having to restart the JVM just to have the HTTP/2 upgrade to be 
attempted again, seems rather harsh.


* HttpHeaders: why is the method "firstValueAsInt" and not 
"firstValueAsLong"? If I want to download e.g. the .iso file of Oracle 
Linux, the value of Content-Length will be too big and a 
NumberFormatException will be thrown.


* HttpHeaders: why is there only a special case for "firstValueAsInt"? 
Personally, I would like some more methods to be added:
** java.time.OffsetDateTime firstValueAsDate(String name, 
Supplier defaultValue) which parses headers such as 
Expired & Last-Modified, using the datetime format recommended by RFC 
7231 (i.e. the format defined in RFC 5322)
** javax.activation.MimeType firstValueAsMimeType(String name, 
Supplier defaultValue) which parses headers such as 
Content-Type & Accept-Patch
** List allCookies() which returns a list of all 
cookies


* HttpHeaders: for the proposed first*() methods, please also consider 
the signature: Optional firstValueAsT(String name). This way, the 
application developer can decide whether to use a default value or to 
throw an exception (or anything else) when the header is absent.


* HttpRequest/HttpResponse: why weren't the static methods added to the 
interfaces 
(HttpRequestBodyProcessor/HttpResponseBodyProcessor/MultiResponseProcessor) 
instead? In my opinion, this would be more logical, and would simplify 
the API of HttpRequest/HttpResponse.


* HttpRequest.Builder: add validation to header/method/setHeader 
(according to the rules in RFC 7230) & throw an IllegalArgumentException 
if validation fails


* HttpRequest.Builder: for "method", how is the parameter handled? Since 
method is case-sensitive ( cf. 
https://tools.ietf.org/html/rfc7230#section-3.1.1 ), creating a request 
with "delete" may fail, simply because it should be "DELETE". I think it 
would be good to at least add a Javadoc note about the case-sensitivity, 
and that the standardized methods ( 
http://tools.ietf.org/html/rfc7231#section-4.1 ) are defined as 
all-uppercase.


* HttpRequest: I'd propose to rename fromByteArray(Iterator) to 
fromByteArrays


* HttpRequest::fromString and HttpResponse::asString should take a 
java.nio.charset.Charset as parameter


* HttpResponse: in my opinion, "asFile" should take a Path as parameter 
& the parameter name should be "file", instead of "filename"


* HttpResponse: concerning the "asString" methods: they refer to "the 
character set specified in the Content-encoding response header". 
However, this should be "the character set specified in the charset 
paramet

Re: Problem with fix B6369510 for HttpURLConnection Content-Type

[Anthony Vanelverdinghe]

Hello

As far as I can see, SCEP is not an RFC yet, but still an Internet Draft 
[1]. In Appendix F, it says that a POST message contains "binary PKCS#7 
data". It does indeed not mention an explicit Content-Type, but this 
seems no more than an oversight: the focus of the document is on doing 
SCEP requests via GET, in which case a Content-Type header is not 
needed. It 's not because the draft doesn't mention a Content-Type, that 
it expects the header to not be set. Actually, as the HTTP RFC says, it 
really should be set: "Any HTTP/1.1 message containing an entity-body 
SHOULD include a Content-Type header field defining the media type of 
that body." [2]
Now I don't know what the correct Media Type is for "binary PKCS#7 
data", but there is e.g. application/pkcs7-mime & 
application/pkcs7-signature. And setting it to 
"application/octet-stream" ought to work anyhow (again from the HTTP 
RFC: "If and only if the media type is not given by a Content-Type 
field, the recipient MAY attempt to guess the media type via inspection 
of its content and/or the name extension(s) of the URI used to identify 
the resource. If the media type remains unknown, the recipient SHOULD 
treat it as type "application/octet-stream".)
As a conclusion, I believe the SCEP draft should be updated to mention 
the correct Content-Type in case of POST requests.


[1] http://datatracker.ietf.org/doc/draft-nourse-scep/?include_text=1
[2] http://tools.ietf.org/html/rfc2616#section-7.2.1

Kind regards, Anthony

Op 27/03/2013 21:55, Matthew Hall schreef:

But the SCEP RFC expects it to be sent without any header. How is JSCEP 
supposed to do this if Java overrides it with no way to prevent the override?

Re: Problem with fix B6369510 for HttpURLConnection Content-Type

[Anthony Vanelverdinghe]

Hello

I don't see any issues with the bug, fix, and test:
before the bug, the header was set for all but PUT requests (cfr. the 
evaluation)
then it was reported this should not be done for GET requests, and the 
evaluation agreed on this,
so the test makes sure GET requests don't have this header set anymore, 
while POST requests still do


I believe the current behavior of setting a default Content-Type for 
POST requests is correct & even desired. Moreover, many Java 
applications do POST requests without explicitly setting the 
Content-type, thereby depending on the default of 
"application/x-www-form-urlencoded" being set.


In my opinion, this is a bug in JSCEP, which does not set the 
Content-Type itself. If the content-type is not 
"application/x-www-form-urlencoded", then JSCEP should set it to 
whatever value is appropriate.


Kind regards

  Anthony


Op 27/03/2013 18:25, Rob McKenna schreef:

HI Matthew,

On the face of it this makes sense. I don't have time to dig into it 
this week, but I'll get stuck into it next week and get a fix together.


-Rob

On 27/03/13 00:42, Matthew Hall wrote:

Forgot to include, offending code in HttpURLConnection:

if (!method.equals("PUT") && (poster != null || streaming()))
 requests.setIfNotSet ("Content-type", 
"application/x-www-form-urlencoded");


Format adjusted a bit for readability.

Matthew.

On Tue, Mar 26, 2013 at 05:33:15PM -0700, Matthew Hall wrote:

Hello,

I was working on a situation which was similar to the situation 
described in

this bug which was supposedly fixed in Java 5 and Java 6:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6369510

The bug described how Content-Type was being auto-set to
application/x-www-form-urlencoded in cases where it should not be.

I was seeing this problem, where the open-source JSCEP library was 
creating a
request to a Tomcat servlet I am implementing, which Tomcat was 
rejecting due

to encoding issues in the POST body.

When I looked at the traffic using Wireshark TCP Stream reassembly I
discovered that the request had the URL-encoded content type even 
though no

code in JSCEP requested this.

Upon reading through the unit test,
openjdk-7/jdk/test/sun/net/www/protocol/http/B6369510.java, I found 
a couple

of issues:

1) The test fails if you have an IPv6 address configured on the system,
because the code doesn't enclose the IPv6 literal with '[]':

URL url = new URL("http://"; + address.getHostName() + ":" + 
address.getPort() + "/test/");


java.net.MalformedURLException: For input string: "0:0:0:0:0:0:0:40392"
 at java.net.URL.(URL.java:601)
 at java.net.URL.(URL.java:464)
 at java.net.URL.(URL.java:413)
 at B6369510.doClient(B6369510.java:63)
 at B6369510.(B6369510.java:52)
 at B6369510.main(B6369510.java:45)

2) There appears to be a logic error in the test, or the fix and the 
bug

description do not match:

if (requestMethod.equalsIgnoreCase("GET") && ct != null &&
 ct.get(0).equals("application/x-www-form-urlencoded"))
 t.sendResponseHeaders(400, -1);

else if (requestMethod.equalsIgnoreCase("POST") && ct != null &&
!ct.get(0).equals("application/x-www-form-urlencoded"))
 t.sendResponseHeaders(400, -1);

This code is saying, the unit test will fail if the method is GET, 
and the
content-type is equal to url-encoded, and the unit test will fail if 
the

method is POST, and the content-type is *NOT* equal to url-encoded.

But, in the evaluation, the bug states, "Content-Type is being set to
application/x-www-form-urlencoded for all HttpURLConnections 
requests other
than PUT requests. This is not necessary and could even cause 
problems for
some servers. We do not need to set this content-type for GET 
requests."


To me this means, the code should not be setting the Content-Type to 
anything,
on any type of request, because it will cause problems across the 
board.


So I think that the test and the bug fix do not actually fix the 
original bug
correctly, and the test needs to be fixed so it will work right on 
an IPv6

based system.

Thoughts?
Matthew Hall.

Re: Http client API

[Anthony Vanelverdinghe]

Hi

With the current API (java.net.HttpURLConnection) it 's not possible to 
follow redirects from one protocol to another (http to https & vice versa).
This is a known problem ( 
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4620571 ), but out of 
security concerns this feature was not added.


Will you please reconsider this feature for the new API and possibly:
add extra methods: 
HttpRequest#setFollowRedirectsAccrossProtocols(boolean follows) & 
HttpRequest#followRedirectsAcrossProtocols() which would be false by default
or add a system property (like the ones at 
http://docs.oracle.com/javase/7/docs/technotes/guides/net/properties.html )

?

I am not a security expert, but for example Firefox happily follows such 
redirects, even in a single request like: http (request) -> https -> 
http (response)
The current behavior is also what caused a recent issue with the JavaFX 
installer ( http://javafx-jira.kenai.com/browse/RT-21275 ). The solution 
to this JavaFX issue says the fix "enhanced code to follow https 
redirects." So JavaFX seems to already implement this feature.


Thanks for your feedback

  Anthony Vanelverdinghe


Op 8/08/2012 1:09, Michael McMahon schreef:

Hi,

A new revision of the Http client API planned for jdk 8 can be viewed
at the following link

http://cr.openjdk.java.net/~michaelm/httpclient/v0.3/

We would like to review the api on this mailing list.
So, all comments are welcome.

Thanks
Michael McMahon.