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