Hi Chris,
> On 31 Mar 2016, at 17:35, Chris Hegarty <chris.hega...@oracle.com> wrote:
>
> I’ve looked at the API a number of times, and overall I’m happy with it
> ( modulo the known open issues ). Some initial comments.
Thanks a lot for looking into this!
> Generally, I prefer javadoc style comments on members. Even it they are
> not public, but that could be just me.
I will think about it.
> In addition of the comment regarding more detailed exception messages,
> maybe a little more detail in assert message would be good, e.g.
> MessagePublisher - assert offered : backlog.size();
I will think about it even harder.
> WebSocketBuilderImpl.java
>
> - forbidden headers could use a TreeSet with a
> String.CASE_INSENSITIVE_COMPARATOR
> and avoid toLowerCase ( and remove your TODO comment ;-) )
It will surely decrease the amount of ugliness in this part of the
implementation, but what I actually meant by this TODO (though apparently
haven't expressed that clearly enough) is that the builder knows (even with
TreeSet with a String.CASE_INSENSITIVE_COMPARATOR) that headers are case
insensitive. Ideally I would delegate the whole task to HttpClient subsystem.
Like here's the set of not allowed headers. And here's a header that a user
wants to set. Please check whether they can do that. This way the builder
won't have to know anything about how headers are compared for equality.
> OpeningHandshake.java
>
> - Are you going to add the URL permission check ?
Thanks for noticing this! That was on my TODO list. And I have almost forgotten
about it. Moreover I will also have to provide a similar solution to
HttpClient's java.net.http.ExecutorWrapper
As HttpClient doesn't share its goodies:
@Override
public ExecutorService executorService() {
return executor.userExecutor();
}
> - the use of a private method, prepareForRequest, in the constructor
> means that nonce and subprotocols cannot be final. Maybe it doesn’t
> matter, but the private method gains you little over a comment and
> inlining the method body.
Yeah... too bad I have to choose between readability and immutability here.
Immutability it is!
> - // FIXME: ensure there's exactly 1 header with this name
> A simple size() == 1 check should suffice.
You see there'a thing. That's why I didn't do it immediately. For example let's
consider a "Sec-WebSocket-Extensions" (a very good illustrative example.)
RFC 6455 states it very clearly [1]:
The |Sec-WebSocket-Extensions| header field MAY appear multiple times
in an HTTP request (which is logically the same as a single
|Sec-WebSocket-Extensions| header field that contains all values.
However, the |Sec-WebSocket-Extensions| header field MUST NOT appear
more than once in an HTTP response.
So looks like there's a difference between:
Sec-WebSocket-Extensions: a, b
and
Sec-WebSocket-Extensions: a
Sec-WebSocket-Extensions: b
RFC forbids the latter one.
Unfortunately I can't tell apart these two cases by simply reading the number of
elements a particular header has. And as I understand, right now I don't have
simple means to do so. In order to do so I'll have to gain access to finer
grained model of headers in the request.
The solution suggested by you would perfectly work for "Sec-WebSocket-Protocol"
and "Sec-WebSocket-Accept". Headers that semantically carry a single atomic
value. But not for "Sec-WebSocket-Extensions" or "Sec-WebSocket-Version".
Btw, I noticed "Sec-WebSocket-Version" doesn't have this FIXME comment, though
it should. Thanks!
> WebSocketImpl.java
>
> - v? maybe find a better name, stateVisitor ?
ok
> - We flip-flopped on the constraint of a single outstanding write, which
> is still in the API, but MessagePublisher has a default of 10 permits?
D'oh! Leftovers from a testing build. Thanks.
> MessagePublisher.java
>
> - this::react feels like a sneaky way of exposing a PRIVAT,
> not-really-private, method.
Sorry, I don't think I understand what you mean. Could you please try to
re-explain it? As I see it now, it's not any different from creating an
anonymous class that has a special trusted relationship with it's enclosing
class.
MessageSender (MS) owns a SignalHandler (SH). MS exposes its part to SH:
private void react()
> I’m still reviewing, and will send more comments later.
Looking forward to hear from you! Thanks again!
--------------------------------------------------------------------------------
[1] https://tools.ietf.org/html/rfc6455#section-11.3.1
