RFR: 8087112 HTTP API and HTTP/1.1 implementation
Michael McMahon
michael.x.mcmahon at oracle.com
Mon Feb 22 16:08:14 UTC 2016
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