RFR: 8087112 HTTP API and HTTP/1.1 implementation
Anthony Vanelverdinghe
anthony.vanelverdinghe at gmail.com
Sun Mar 20 22:35:11 UTC 2016
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<HttpResponse> 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<T extends ByteBuffer> extends
Flow.Processor<T, T> {}
interface BodyHandler {
BodyProcessor<?> apply(URI uri, String method, Http.Version
version, HttpHeaders headers);
}
in HttpResponse:
interface BodyProcessor<T extends ByteBuffer, R> extends
Flow.Processor<T, T>, Supplier<Future<R>> {}
interface BodyHandler<R> {
BodyProcessor<?, R> 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 request/response-specific
information which may be required, is provided by the BodyHandler
implementation.
The BodyHandler can easily be made thread-safe, and thus shareable. It
can also decide whether to return a new BodyProcessor instance each
time, or return a single shared instance instead.
The library itself, would also use a Flow.Processor to interact with the
BodyProcessors.
For a HttpRequest.BodyProcessor, this would work as follows:
- the BodyProcessor fills & publishes ByteBuffers to the library
- the library reads them & in return publishes them to the
BodyProcessor, where they can be recycled
For a HttpResponse.BodyProcessor, this would be analog:
- the BodyProcessor publishes empty ByteBuffers to the library
- the library fills them & in return publishes them to the
BodyProcessor, where they can be read & recycled
This mechanism allows a BodyProcessor to cancel the operation, simply by
stopping the publishing of ByteBuffers, e.g. with an
onError(cancellationException).
This could easily be extended to work with a ByteBuffer pool, where the
pool implements Flow.Publisher<ByteBuffer>, and the BodyProcessor's
Publisher delegates to it:
Pool -> BodyProcessor -> library -> BodyProcessor -> Pool
(Note that the final step of returning ByteBuffers to the Pool is a
simple method invocation, not a Flow.)
I have actually implemented a proof of concept of this, with a shared
pool etc., so I can say it all works in practice.
=== "HttpStream"
I believe there's a missing abstraction, between HttpClient and
HttpRequest: the concept of a logical connection between client and
server, a "HttpStream".
I'm not saying this abstraction should be introduced this late in the
game, but rather that the API must make sure not to contain any
stream-level concepts for now. The abstraction could then possibly be
added in a future enhancement.
Therefore, the following should be removed from HttpClient[.Builder]:
- pipelining: this is unsupported anyway
- priority: there's no concept of streams and no way to declare
dependencies between streams, so I don't see how this can be currently
useful. It should be possible for a single HttpClient to have multiple
streams to the same origin, but with different priorities.
Something else that would be on the stream-level, is a way to disable
push (using SETTINGS_ENABLE_PUSH).
=== HTTP headers
It should be clearly stated in the Javadoc of HttpClient and/or
HttpRequest that the HttpClient has full control over the headers which
actually get sent. And that e.g. the use of Authenticator and
CookieHandler is required, i.e. developers cannot manually set an
Authorization or Cookie header, respectively.
In my opinion there should also be a programmatic way to access the
exact headers that are sent (at least whenever an exception occurs, for
debugging purposes. To this goal I'd introduce a HttpException class,
which contains: client, request, raw request headers).
As for the currently disallowed headers, I believe there are some
headers which shouldn't be, namely: Date, Expect, From, Origin, Referer,
User-Agent, Warning
and some which should be: HTTP2-Settings, TE
- Date
this header should only be set if the application believes it's useful.
It shouldn't be systematically done by the HttpClient. From the RFC [2]:
"A user agent MAY send a Date header field in a request, though
generally will not do so unless it is believed to convey useful
information to the server."
- Expect
this is a generic header, which could be used to convey
application-specific expectations
- From/Origin/Referer/User-Agent
these all provide similar information, and I don't see any reason why
they should be disallowed
- Warning
this is a response header
- TE
this is something that should be handled transparently by the library
(cf. my proposed handling of Transfer-Encoding below). Either way, since
it requires the "TE" option to be set in the "Connection" header [3],
it's simply impossible to correctly set the TE header.
To summarize, I believe the set of disallowed headers must be:
- any headers defined in RFC7230 and RFC7540, i.e. low-level,
connection-specific headers, which the HttpClient should handle
transparently
- any headers defined in RFC7235, since the HttpClient's Authenticator
must be used instead
- the Cookie header, since the HttpClient's CookieHandler must be used
instead
=== Transfer-Encoding
I believe this should be handled by the implementation, amongst others
because:
- it requires the TE header to be set, which is impossible for the
application to do, since doing so requires setting the corresponding
option in the "Connection" header. However, that header is unmodifiable
by the application.
- since the implementation must set the TE request header itself, it
follows that it should also handle the corresponding response headers
- it's a low-level, connection-specific header
- the implementation already handles chunked Transfer-Encodings
- there are only 3 current encodings (gzip, compress, deflate), of which
both gzip and deflate could be readily supported by using the
InputStream implementations in java.util.zip (which is in java.base)
- this would transparently give a performance boost to any application
requesting highly-compressable resources, such as text files
=== Concurrent connections
The implementation should make sure that concurrency is within
reasonable bounds [4]. These limits must be global, across all
HttpClients in the current JVM. Ideally, it should also make these
configurable, for example through the following system properties
(inspired by Firefox' about:config):
java.net.http.maxConnections = 256
java.net.http.maxPersistentConnectionsPerProxy = 32
java.net.http.maxPersistentConnectionsPerOrigin = 6 (with origin as
defined in RFC6454 [5])
=== HttpClient
The way automatic redirects are handled, doesn't allow customized
redirection policies.
=== HttpRequest
An instance is currently tied to a specific HttpClient. In my opinion,
this is unnecessarily restricting.
=== HttpRequest.BodyProcessor
onStart returns the Content-Length, but:
- it's unreliable
For example the current "fromFile" BodyProcessor returns "fc.size()".
Now if the file is modified while the body is processed, a different
number of bytes will be passed to the implementation. Either the
implementation will detect this & throw an error straightaway, or the
server will return a "client error" status code. This case is not as
uncommon as it seems, e.g. say I want to upload a log file of an active
process, which continuously appends new lines to it.
- it's kind of useless if the library is still to apply some
Transfer-Encoding afterwards
- it doesn't help in deciding whether or not to send with chunked
Transfer-Encoding (though, if I'm not mistaken, the current
implementation uses it as its sole criterion to do so)
For example, if a BufferedImage is to be uploaded (encoded on-the-fly
as image/jpg), the exact Content-Length is unknown, but this should not
be chunked, as all the data is readily available in memory.
On the other hand, if I have a 4GB .iso image to upload, the
Content-Length is known, but this should be chunked
I believe a better criterion would be, to both set a timer and have a
buffer: if either the buffer overflows, or the timer goes off, use
chunked TE & start sending.
=== HttpRequest.Builder
- currently, "HttpClient.getDefault().request().GET()" throws a
NullPointerException without a message. An IllegalStateException
referring to the uri would be more appropriate.
- there ought to be a way to create a Builder from a HttpRequest.
Actually, I think this would be much more common than wanting to copy a
Builder, so I'd remove the copy() method in Builder and introduce a
"Builder with()" instance method on HttpRequest.
- there ought to be a separate build() method which does the validation
and final creation, instead of the method-related methods:
currently, if you want to create a copy with the same method as the
original (which seems a desirable thing), you must first create the
HttpRequest. And even then it's fragile, because the fact that there's
an internal instance variable which gets set at this point (& will thus
get copied along) is merely an implementation detail.
- currently, copy() copies the BodyProcessor as-is. This will not work
if the BodyProcessor isn't both thread-safe and repeatable (which will
almost never be the case). On the other hand, a BodyHandler (as proposed
above) could easily be made thread-safe and shareable.
=== HttpResponse
- body/bodyAsync: these are problematic because:
- if a body is present, one of these should be called. However, this
cannot be enforced and is instead stated as a requirement in the class'
Javadoc
- they must be called in a timely manner: if the client waits too
long, the server will have closed the connection already (cf. the bug at
the beginning)
- they must not be called more than once, and thus synchronized:
on a second invocation with body(), it will hang until the Thread
is interrupted (and this only works by the grace of SocketChannel being
interruptible)
on a second invocation with bodyAsync(), it will hang until the
HttpClient's ExecutorService is shut down
To solve this, I introduced a "HttpResponse<R, Void> send(HttpRequest
r, BodyHandler<R> bh)" method on HttpClient
- uri: this API is too limited, in my opinion. It should 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<Path, Path>, instead of the current Map<URI, Path>:
interface MultiResponse<T, U> {
T mainResponse();
ConcurrentMap<U> 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<R, S> send(HttpRequest r,
BodyHandler<R> bh, PushResourceHandler<S> 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<HttpResponse<?, ?>, Integer,
Optional<HttpRequest>> 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 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<ByteBuffer>
>> publisher);
>>
>> }
>>
>> and an example implementation:
>>
>> class FromString implements HttpRequest.BodyProcessor {
>>
>> final String s;
>>
>> FromString(String s) {
>> this.s = s;
>> }
>>
>> void send(HttpRequest request, SubmissionPublisher<ByteBuffer>
>> publisher) {
>> Optional<Charset> 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<T> extends
>> Flow.Subscriber<ByteBuffer> {
>>
>> Optional<T> beforeReceipt(long contentLength, int statusCode,
>> HttpHeaders responseHeaders) throws IOException;
>>
>> T afterReceipt() throws IOException;
>>
>> }
>>
>> and an example implementation:
>>
>> class AsFile implements HttpResponse.BodyProcessor<Path> {
>>
>> final Path p;
>> FileChannel fc;
>> IOException e;
>> Flow.Subscription subscription;
>>
>> AsFile(Path p) {
>> this.p = p;
>> }
>>
>> Optional<Path> 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<Map.Entry<HttpResponse, List<HttpResponse>>>
>> responseAsync(BiFunction<HttpRequest, HttpHeaders, Boolean>
>> pushHandler) { ... }
>>
>> Moreover, response() and responseAsync() could then easily be
>> implemented in terms of this method, as follows:
>> HttpResponse response() { return responseAsync().get(); }
>> CompletionStage<HttpResponse> 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<Duration> 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<HttpResponse>
>> 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 sslParameters()", with
>> default value sslContext().getDefaultSSLParameters()
>> --- use a sensible default where possible
>> - version(): change to "HttpVersion version()"
>>
>> HttpClient.Builder
>> - add "HttpClient.Builder timeout(Duration timeout)"
>> - cookieManager(CookieManager manager): change to
>> cookieHandler(CookieHandler handler)
>> - followRedirects(HttpClient.Redirect policy): change to
>> redirectIf(Predicate<HttpResponse> policy)
>> - pipelining(boolean enable): remove
>> - priority(int priority): remove
>> --- unused, can't be set per-request, unable to express stream
>> dependencies
>> - version(HttpClient.Version version): change to version(HttpVersion
>> version)
>>
>> HttpClient.Redirect
>> - replace with top-level "enum StandardRedirectionPolicies implements
>> Predicate<HttpResponse>" with 4 constants: TRUE, FALSE, SECURE,
>> SAME_PROTOCOL
>> --- analog to java.nio.file.StandardOpenOption, allows to implement
>> custom redirection policies, while providing standard policies for
>> the common use-cases
>>
>> HttpClient.Version
>> - move to top-level class HttpVersion
>> --- not client-specific
>>
>> HttpHeaders
>> - firstValueAsLong(String name): change to "OptionalLong
>> firstValueAsLong(String name)"
>>
>> HttpRequest
>> - add "HttpRequest.Builder create()"
>> --- for consistency with HttpClient.request()
>> - add "ProxySelector proxy()"
>> - add "Optional<Duration> timeout()"
>> - followRedirects(): change to "Predicate<HttpResponse>
>> redirectionPolicy()"
>> - fromXxx(): move to HttpRequest.BodyProcessor
>> --- HttpRequest API becomes cleaner, and aids in discoverability to
>> find implementations in the interface itself
>> - fromByteArray(byte[] buf, int offset, int length): remove
>> --- easily done with fromByteArray(new ByteArrayInputStream(buf,
>> offset, length))
>> - fromByteArrays(Iterator<byte[]> iter): replace with
>> "fromByteArrays(Stream<byte[]> stream)"
>> - fromString(String body): converted using the character set as
>> specified in the "charset" parameter of the Content-Type HTTP header
>> --- from RFC 7231: "The default charset of ISO-8859-1 for text media
>> types has been removed; the default is now whatever the media type
>> definition says."
>> - fromString(String s, Charset charset): remove
>> --- to ensure consistency with the "charset" parameter of the
>> Content-Type HTTP header, don't allow to pass a charset here
>> - multiResponseAsync(HttpResponse.MultiProcessor<U> rspproc): replace
>> (cf. proposals)
>> - noBody(): move to HttpRequest.BodyProcessor
>> - responseAsync(): change return type to CompletionStage<HttpResponse>
>> --- the caller shouldn't be able to complete the CompletableFuture
>> itself
>>
>> HttpRequest.Builder
>> - add "HttpRequest.Builder DELETE()"
>> --- often used with "RESTful APIs". Moreover, PUT was already added
>> - body(HttpRequest.BodyProcessor reqproc): change to
>> body(Supplier<HttpRequest.BodyProcessor> reqproc)
>> --- to allow copy: copying the BodyProcessor itself would cause
>> concurrent calls to methods of the same BodyProcessor instance
>> - followRedirects(HttpClient.Redirect policy): change to
>> redirectIf(Predicate<HttpResponse> policy)
>> - header(String name, String value): change to header(String name,
>> UnaryOperator<List<String>> valueUpdater)
>> --- allows to replace setHeader, and is much more flexible, e.g. can
>> be used to remove values
>> - headers(String... headers): change to headers(Map<String, String>
>> headers)
>> - setHeader(String name, String value): remove
>> - timeout(TimeUnit unit, long timeval): change to timeout(Duration
>> timeout)
>>
>> HttpResponse
>> - asXxx(): move to HttpRequest.BodyProcessor
>> - asByteArrayConsumer(Consumer<byte[]> consumer): replace with
>> "static HttpResponse.BodyProcessor<Stream<byte[]>> asByteArrays()"
>> - asString(): specify that if the Content-Type header does not have a
>> "charset" parameter, or its value is unsupported, an exception is
>> thrown; replace reference to Content-encoding with Content-type
>> - asString(Charset charset): replace with asString(Charset charset,
>> boolean force); replace reference to Content-encoding with Content-type
>> --- if "force" is false, only use if Content-type's charset parameter
>> is not available. If "force" is true, always use the given charset
>> - body(HttpResponse.BodyProcessor<T> processor): change to
>> body(Supplier<HttpResponse.BodyProcessor<T>> processorSupplier)
>> --- such that the library has the ability to create BodyProcessors
>> itself, e.g. in case of a retry. It also shows the intention that
>> processors should typically not be shared
>> - bodyAsync(HttpResponse.BodyProcessor<T> processor): change to
>> "CompletionStage<T> bodyAsync(Supplier<HttpResponse.BodyProcessor<T>>
>> processorSupplier)"
>> --- prevent caller from completing the CompletableFuture itself +
>> same as body()
>> - multiFile(Path destination): move to HttpResponse.MultiProcessor
>> - sslParameters(): remove
>> --- cannot be HttpRequest-specific, and thus can always be retrieved
>> as request().client().sslParameters()
>>
>> HttpRequest.BodyProcessor
>> HttpResponse.BodyProcessor
>> HttpResponse.MultiProcessor
>> - cf. proposals above
>>
>> HttpTimeoutException
>> - HttpTimeoutException(String message): make package-private
>> --- in case you'd want to add a public method such as "HttpRequest
>> request()" in a future release, you could then just add a new
>> package-private constructor with a HttpRequest parameter, and
>> guarantee request() would never return null
>>
>>
>> And on the implementation (I just skimmed through the code):
>> java.net.http
>> - running NetBeans' analyzers on java.net.http gives 143
>> warnings/errors, so it would be good to fix those that need fixing
>> - declare HashMap and LinkedList variables as Map and Queue/List
>> wherever possible
>>
>> AuthenticationFilter
>> - use HttpUrlConnection.UNAUTHORIZED / HTTP_PROXY_AUTH instead of
>> hard-coded 401 / 407
>>
>> BufferHandler
>> - why not named ByteBufferPool, analog to ConnectionPool?
>>
>> Exchange/ExchangeImpl
>> - it's confusing that ExchangeImpl isn't a subclass of Exchange
>>
>> Http1Request
>> - collectHeaders contains: URI uri = request.uri(); [...] if (request
>> == null) {
>>
>> HttpClientImpl
>> - starting the selmgr Thread in the constructor is unsafe
>> - there's more synchronization than is required, since there are 2
>> independent groups of methods: 1 for timeouts & 1 for freelist
>> - the default client's ExecutorService doesn't use daemon threads,
>> while a client without explicit ExecutorService does
>>
>> HttpConnection
>> - contains hard-coded Unix path: new
>> FileOutputStream("/tmp/httplog.txt")
>>
>> HttpRequestBuilderImpl
>> - copy: should clone userHeaders
>>
>> HttpRequestImpl
>> - DISALLOWED_HEADERS: why is "user-agent" disallowed? and "date"
>> (which is a response header)?
>>
>> Pair
>> - use Map.Entry and the factory method Map.entry() instead
>>
>> SSLTunnelConnection
>> - connected(): "&" should be "&&"
>>
>> Utils
>> - BUFSIZE: why "Must never be higher than 16K."?
>> - validateToken: must handle token being null or empty, must accept
>> single quotes, must reject parentheses
>> - copySSLParameters: move to SSLDelegate
>> - unflip/compact/copy: remove
>>
>>
>> Finally, here's some issues I noticed while trying it out:
>>
>> Every request seems to include the "?" of the URI's query part.
>>
>> The following is a simple server and client, where the server stops
>> as soon as it has read a request.
>> If you start the server, then start the client, the client will throw:
>> java.net.http.FatalIOException: Retry limit exceeded
>> however, there are neither suppressed exceptions, nor a cause. Also,
>> the exception on the retries looks suspicious:
>> java.io.IOException: wrong content length
>> at
>> java.net.http.Http1Request.writeFixedContent(Http1Request.java:320)
>>
>> import com.sun.net.httpserver.HttpServer;
>> import java.io.IOException;
>> import java.net.InetSocketAddress;
>>
>> public class SimpleServer {
>>
>> public static void main(String[] args) throws IOException {
>> HttpServer server = HttpServer.create(new
>> InetSocketAddress(8080), 0);
>> server.setExecutor(null);
>> server.createContext("/", e -> {
>> e.getRequestBody().readAllBytes();
>> server.stop(0);
>> });
>> server.start();
>> }
>>
>> }
>>
>> import java.io.IOException;
>> import java.net.URI;
>> import java.net.http.HttpRequest;
>>
>> public class SimpleClient {
>>
>> public static void main(String[] args) throws IOException,
>> InterruptedException {
>> HttpRequest.create(URI.create("http://localhost:8080"))
>> .body(HttpRequest.fromString("helloworld"))
>> .POST()
>> .response();
>> }
>>
>> }
>>
>> Kind regards,
>> Anthony
>>
>>
>> On 4/02/2016 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/
>>>
>>> The HTTP/2 implementation will come later.
>>>
>>> Thanks,
>>> Michael.
>>
>
More information about the net-dev
mailing list