Hi Roger, thanks for looking into this.
> On 5 Apr 2016, at 17:37, Roger Riggs <roger.ri...@oracle.com> wrote:
>
> It would be helpful if the classnames/filenames reflected the participation
> in the WebSocket implementation
> to keep them distinct from the HTTP 2.0 implementation in the same directory.
> For example, Writer, Reader, etc. perhaps a 'Ws' prefix would be sufficient.
>
> This would be alleviated if websocket was in its in its own package.
> java.net.ws
> (though I realize that may already have been settled, and I'm sure there are
> some dependencies
> that would be nice to keep as package private).
We've been through this. WebSocket has a very tight relationship with the new
HttpClient. It's more like an addition to it rather than a standalone API.
Having said this, the consensus was there's little justification for it to be in
its own API package.
That doesn't mean though we can't put the _implementation_ in its own package
inside java.httpclient module. That would make the implementation look a lot
cleaner.
> Logging is sprinkled throughout the APIs of the implementation; it would be
> cleaner to have
> a shared WS logging class with a static logger similar to the Http Log (or
> perhaps some sharing and reuse)
> so the logging does not clutter the implementation interfaces. It would rare
> that separate logger
> instances are needed.
I tried hard to move higher level logging to a separate decorators, so the
implementation code wouldn't have to bother (see
java.net.http.LoggingWebSocketFactory).
Unfortunately, sometimes I have to mix logging code with the payload code in
order to log the actual processing and not just the input parameters.
I agree I should use statically imported log instance everywhere. But at the
same time I would prefer to keep logging calls as they are today. In other
words, directly to the log instance instead of to some facade. I don't see much
difference right now between
Log.logTrace("cancelling stream: %s\n", e.toString());
and
log.log(TRACE, "cancelling stream: %s\n", e.toString());
Sometimes I pass java.lang.System.Logger as a parameter to the code that might
be shared in the future with HttpClient (e.g. java.net.http.Shared, sorry for
the funny naming coincidence in this case).
> Writer.java:
>
> - line 143: keep track of the current/next buffer index of the first
> non-empty buffer; instead of replacing with NULL_BYTE_BUFFER;
Thanks.
> a combination of the buffers array index vs length and hasRemaining could
> eliminate the need to copy the buffers array
> and perform the initialization to determine the total length (though that is
> informative for logging)
> - not sure why you are using the 3 arg form of GatheringWriter since the
> byteBuffers array is exactly the length.
Sorry, I don't think I understand. There's no need to copy. There's a need to
map Shared<ByteBuffer>[] -> ByteBuffer[] in order to provide a correspondence
between buffers and their enclosing containers for the sake of disposing
the buffers that have been fully written from.
> - line 184: editorial, remove "from" from the logger
Thanks.
> - line 228: can the Throwable be discarded? Does it contain no information?
I have to think about it. Writer is a Subscriber. So whatever happens in the
corresponding Publisher (MessageSender) should probably be logged by only one of
them. People generally don't like to see echoed stacktraces. There seems to be
no information for Writer except for there won't be any more onNext.
> Reader.java:
> - line 137: Why should the reader force a new task? Should it be left to
> the subscriber to
> decide whether its processing is lightweight and can be done immediately
> or to re-submit to its own executor.
I was thinking about getting Reader back to reading ASAP, and not _potentially_
wasting its time in the subscriber. I might rethink this. It is a decoupling
obsession :-)
Thanks for pointing this out.
> java.net.Utils:
> - line 231: SATURATING_INCREMENT (and its uses) is wasted code; long is not
> going to roll over.
It's in order to go with the flow :-) I mean Flow/Reactive Streams API. I
believe it makes the implementation more robust. As [1] mentions demand greater
than Long.MAX_VALUE. It's a corner case anyway.
Would you suggest throwing an IAE in case where the implementation detects
overflow?
> - line 236: NULL_BYTE_BUFFER isn't null - prehaps rename to EMPTY_BYTE_BUFFER
Thanks. It's been already mentioned by Andrej Golovnin. The name is bad, sorry
about that (though I referred to the Null Object pattern, rather than to null
literal).
I don't think EMPTY is a perfect name. What is an *empty* ByteBuffer? In my
opinion it's not necessarily the one that has a capacity of 0. It's the one that
reports hasRemaining() == false. I want to reflect both qualities in the
buffer's name. But maybe I'm overcomplicating this naming thing.
Thanks.
--------------------------------------------------------------------------------
[1]
https://github.com/reactive-streams/reactive-streams-jvm/#3-subscription-code
rule #17
