JEP 321 surface-level comments

[Chris Povirk]

Hi,

I'm one of the people on the Java Team at Google. We got asked to have a
quick look at the JEP 321 HTTP Client API. Here are some fairly
surface-level thoughts. I hope that they're helpful (and I'm sorry that
they're so late).

(Also: After writing my first draft of comments, I skimmed through the
previously filed issues and some of Tobias Thierer's comments, hoping to
avoid avoid unintentionally duplicating things you've already covered. But
I may still duplicate some. Certainly don't feel the need to rehash
anything on my behalf.)

   - 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.)
   - 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.
   - 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).
   - 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.)
   - In the HttpRequest.Builder docs, it would be nice to call out that GET
   is the default.

--Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20180508/e8faa38c/attachment.html>