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 bugThe 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 headersIt 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 - Datethis 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."
- Expectthis is a generic header, which could be used to convey application-specific expectations
- From/Origin/Referer/User-Agentthese all provide similar information, and I don't see any reason why they should be disallowed
- Warning this is a response header - TEthis 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-EncodingI 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 connectionsThe 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 = 32java.net.http.maxPersistentConnectionsPerOrigin = 6 (with origin as defined in RFC6454 [5])
=== HttpClientThe way automatic redirects are handled, doesn't allow customized redirection policies.
=== HttpRequestAn 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 unreliableFor 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'schanging 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, wehave 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 beaccommodated, though not necessarily mandated as part of the API given the1 to 1 relationship here, whereas Flow being more 1 to N.I think that by using basically the same type of flow control mechanism meansthat 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 SubmissionPublisherthough. 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 sendingside 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 havequestions) Thanks Michael. On 22/02/16 00:47, Anthony Vanelverdinghe wrote:Hi MichaelHere'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 retriesGiven 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-casesHttpClient.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 itselfHttpRequest.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 nullAnd 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 possibleAuthenticationFilter- use HttpUrlConnection.UNAUTHORIZED / HTTP_PROXY_AUTH instead of hard-coded 401 / 407BufferHandler - 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 doesHttpConnection- 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 exceededhowever, there are neither suppressed exceptions, nor a cause. Also, the exception on the retries looks suspicious:java.io.IOException: wrong content lengthat 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 toplevel 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.
