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.
