Hi Tobias,
On 10/05/18 19:51, Tobias Thierer wrote:
On Tue, May 8, 2018 at 12:35 PM Chris Povirk <cpov...@google.com
<mailto:cpov...@google.com>> wrote:
Commenting inline on just a couple of points.
* I second the idea of making HttpHeaders a final class, as
proposed as part ofJDK-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
foraccept-encoding instead of Accept-Encoding.)
To give more background: The argument of allowing different
implementations that could do stuff such as parse headers lazily was
suggested to me (verbally, I think) by either Chris Hegarty or Michael
McMahon. Like you (Chris Povirk) say, after carefully reading RFC 7541
(HPACK), I didn't find the argument convincing because I didn't see a
way for how header parsing could to a substantial extent be done
"lazily" - any RFC 7541 compliant implementation will need to keep track
of the dynamic table state, at which point there's not much work left
that could be done lazily. I believe I gave this feedback to Chris and
Michael, too (perhaps even on this list).
Already answered in my reply to Chris Povirk.
The additional points you (Chris Povirk) raise about guaranteeing a key
order and consistent behavior with respects to nulls make sense to me.
Since HttpHeaders is a pure value type, I think that further supports
the argument that it should be either a final class, or at least not
allow custom implementations by application developers.
Already answered in my reply to Chris Povirk.
* 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.)
The mix of concerns (business logic vs. value type) did also strike me
as making the API more difficult to use because it closely couples the
business logic and value type semantics - making it harder/impossible
for an application to reimplement e.g. only some business logic. This
was discussed in more detailed feedback that Google provided a long time
ago but it wasn't clear (to me, at least) whether/how we'd be able to
provide additional value beyond making sure that Chris and Michael were
aware of this perspective.
We did discuss this a long time ago, and we addressed the substantive
points at the time, i.e the sendXXX methods were previously virtual
methods on HttpRequest, thus mixing function and state within
HttpRequest. There were other comments, some relating to extensibility,
etc, that were also addressed in the first round of feedback from
Google. The second round of feedback was transcribed into JIRA items,
retrievable at https://bugs.openjdk.java.net/issues/?filter=3D31878
Also, my impression was that Chris and Michael have been trying to
finalize the API for a while and that it might be too late for such changes?
The primary goal of JEP 321 [1] is to standarize the API. The JEP has
been targeted to JDK 11 [2]. The JDK 11 schedule is available on the
JDK 11 release specific section of the JDK project page [3].
-Chris.
[1] http://openjdk.java.net/jeps/321
[2] http://mail.openjdk.java.net/pipermail/jdk-dev/2018-April/001072.html
[3] http://openjdk.java.net/projects/jdk/11/
