JEP 321 surface-level comments

[Chris Hegarty]

Hi Chris, Tobias,

Apologies for the delayed reply. I have spent a significant amount of
time on this feedback, both prototyping and determining any potential
impact on the API. There are a number of proposals in this email that
address some of the comments, as well as explanations for those that do
not require changes. It would be really helpful if you could acknowledge
each, after which I will file JIRA issues and get them resolved.

On 08/05/18 20:34, Chris Povirk wrote:
> ...
> 
>   * I second the idea of making HttpHeaders a final class, as proposed
>     as part of JDK-8184274
>     <https://bugs.openjdk.java.net/browse/JDK-8184274>. While "final
>     class" was part of that suggestion, it doesn't seem to have been
>     specifically shot down. Tobias suggested that the goal of an
>     extensible class was to let users plug in a lazily parsed map for
>     HTTP 2, but he didn't seem to think this was likely to work, given
>     the HTTP 2 spec. The pros I see for a final class, besides the usual
>     advantages of simplicity, are: It could ensure that instances use a
>     Map that retains key order and doesn't contain nulls. It could also
>     implement case insensitivity, which the current implementation
>     claims to do but doesn't. (Maybe it's relying on internal callers to
>     do normalize strings before calling it? But that doesn't help the
>     external caller who asks for accept-encoding instead of
>     Accept-Encoding.)


For HTTP/2, to keep the client and server HPACK indexing tables
consistent it is required that all header block fragments be decoded.
HTTP/1.1, obviously, does not have such a restriction. A lazy parsing
implementation of HTTP/1.1 headers is not likely to be common, but
possible in the current design.

HttpHeaders instances are required, by specification, to be immutable,
but I accept that a final class, and implementation, is a stronger
contract. Ordering of any elements of the HttpHeaders is not something
that we want to guarantee, and would likely lead to hard to diagnose
issues if some aspect of user code depended on it, since the HTTP
protocol does not provide any such ordering guarantees, nor the
iteration order of the returned map.

All that said, on balance I think it may be better to make HttpHeaders
final. Given the requirement for other client implementations to be
able to create an HttpHeaders, then it will need a first-class public
API for its construction, even though direct construction of an
HttpHeaders is not something that we necessarily want to promote in the
API. Rather HttpHeaders are returned from an instance of HttpRequest or
HttpResponse.


Proposal: Make HttpHeaders final, as follows:

  1) Change from abstract to final class ( obviously )
  2) Remove its protected constructor
  3) Remove the "default implementation" notes ( since there will be
     just one implementation )
  4) Provide a public factory method:

    /**
     * Returns an HTTP headers from the given map. The given map's key
     * represents the header name, and its value the list of header
     * values for that header name.
     *
     * @param headerMap the map containing the header names and values
     * @param filter a filter that can be used to inspect each entry in
     *               the given map to determine if it should, or should
     *               not, be added to the to the HTTP headers
     * @return an HTTP headers instance containing the given headers
     * @throws NullPointerException if any of: {@code headerMap}, a key
     *         or value in the given map, or an entry in the map's value
     *         list, or {@code filter}, is {@code null}
     */
    public static HttpHeaders of(Map<String, List<String>> headerMap,
                                 BiPredicate<String, List<String>> 
filter)

After much prototyping, a filter argument in the API will significant
reduce the need to re-creating Maps just for the purpose of feeding
into an HttpHeaders ( and a defensive copy of the Map must be performed
by the factory to ensure structural immutability ). The above also
addresses the nullability concern.

---

 From the HttpHeaders class-level description:

   "The methods of this class ( that accept a String header name ), and
    the Map returned by the map method, operate without regard to case
    when retrieving the header value."

The following demonstrates the case insensitivity.

$ ~/binaries/jdk-11.jdk/Contents/Home/bin/jshell
|  Welcome to JShell -- Version 11-ea
|  For an introduction type: /help intro

jshell> import java.net.http.*;

jshell> URI uri = URI.create("http://foo.com");
uri ==> http://foo.com

jshell> HttpRequest req = 
HttpRequest.newBuilder(uri).header("Accept-Encoding", "gzip, 
deflate").build();
req ==> http://foo.com GET

jshell> HttpHeaders headers = req.headers();
headers ==> jdk.internal.net.http.ImmutableHeaders at d9233844 { ... 
encoding=[gzip, deflate]} }

jshell> System.out.println(headers.firstValue("accept-encoding").get());
gzip, deflate


If you do not find that this is the case, then that's a bug. Please
create a short example, or otherwise describe how to reproduce the
issue you encounter.


>   * The HttpHeaders doc could be clearer about what it does with
>     comma-separated header values. I assume that the caller just gets
>     the whole string as a single value, since HttpHeaders would have to
>     know whether a given header is comma-separated or not, but some
>     users might expect it to be split, especially given the List<String>
>     return type and the interchangeability of comma-separated values and
>     multiple headers in some cases.


HttpHeaders does not do any interpretation of header values.

Proposal: Add a minor specification clarification to avoid any future
potential confusion on this point, such as:

   "Header values are uninterpreted strings."


>   * Could HttpClient drop its getters? They feel like implementation
>     details of the default HttpClient implementation, not properties of
>     an HTTP client in the abstract. (e.g., even if my alternative
>     implementation were to support cookies, it might not use the JDK's
>     cookie API.) It feels kind of like if Future had executor() and
>     runnable() getters: *Many* Futures will have reasonable values for
>     them, but not all, so I'd hope most users would avoid using them,
>     anyway, to be safe. Or, if the getters stick around, I could see
>     putting them on some kind of DefaultHttpClient class (to which the
>     current HttpClient.Builder might move).


The getters are necessary and are part of the abstract client. If one
has an HttpClient, whose construction is unknown, then it may be
necessary to determine the client's configuration. For example, if a
cookie handler has been configured, then the code performing exchanges
through the client need not explicitly process cookie related headers,
that can be left to the cookie handler. Equally, the calling code should
be able to determine if a cookie handler is not configured, so that it
can take appropriate action. Similar with redirects and authentication.

The Executor is a little different. To provide the most flexibility and
allow for better use of resources, to build user-level chains of
asynchronous dependent actions, the executor is required. Again, this is
when the construction-site and use-site of the client do not have
explicit knowledge of each other.

Providing finer grain abstractions than the existing HttpClient seems
not worthwhile.


>   * I'm a little confused by HttpRequest. At first, I thought it was a
>     value type like HttpHeaders, so I was going to have the same
>     suggestion of making it a final type (and also its builder). But the
>     implementation appears to be mutable and to contain additional
>     fields. It feels like the type is pulling double duty as both a
>     value type in the public API (which could be nice to make final) and
>     as a stateful internal type (which might contain an instance of the
>     public type), and it's not clear if different implementations could
>     successfully interchange HttpRequest instances. (Tobias said that
>     he'd raised this point earlier, and it sounds like it may
>     realistically have to stay how it is, but since this was my reaction
>     and it turned out to match what he'd said, I wanted to at least
>     mention it again.)


 From an API perspective HttpRequest is immutable, and all
implementations returned from the client exhibit this behaviour. If not,
then it's a bug. I think, since you clearly looked at the
implementation, that you noticed that the HTTP request implementation
has some mutable state that it carries, that is required for certain
parts of the default implementation. That is not unusual or any cause
for alarm, and does not have any impact on the exposed state.

There can be different HttpRequest implementations. For example, the
default implementation disallows requests with the CONNECT method -
which is used for tunneling - and there are restrictions around its use
when performing security manager permission checks. Another
implementation may not have such a restriction. There are a few other
small constraints and restrictions specific to the default client that
another implementation may choose to do differently.

It is for these reasons that neither HttpRequest or its Builder are
final. More generally, there is a clear tension between extensibility
and finality. We favour the former to be more friendly to other
implementations, and to support scenarios like offline testing ( and to
a lesser extent mocking ).


>   * In the HttpRequest.Builder docs, it would be nice to call out that
>     GET is the default.


Agreed.	The fact that the default request method is GET could be made
clearer.

Proposal: to HttpRequest.Builder add:

   * <p> The builder can be used to configure per-request state, like:
   * the request URI, the request method (default is GET unless
   * explicitly set ), specific request headers, etc.

-Chris.