Hi Pavel,
On 4/5/2016 2:27 PM, Pavel Rappo wrote:
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.
yes, that would be an improvement
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());
Either is fine, if log is a public final static.
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).
I still think this is better as a package private logger; minimally, the
primary arguments of the methods
should be first, and the logger if absolute necessary at the end. It is
a distraction in the implementation.
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.
ok, the next set of comments will make the case that Shared should
extend ByteBuffer. ...
- 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.
Perhaps then make it clear where the exception gets passed back to the
application.
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.
I don't think there's an issue with throughput in the Reader, the OS is
buffering and the work of the
reader is copying from the system's internal buffers to the directbyte
buffer, a cpu bound task.
Also consider that in delivering a buffer to a subscriber the caller has
no idea how much work
the subscriber needs to do; It may be trivial or a lot. In either
case, the subscriber is in the
best position to decide if a separate task is warranted. (At least in
the general case).
Except for the callout to the Listener; all the tasks are wholely known
to the WS implementation.
I think the listener should be given the choice about whether to create
a new task.
The recommendation can be to return from listener callbacks promptly;
but its their app if they don't take the advice.
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?
No, it is NOT going to overflow; it can handle 1Gb/sec for 292 years.
Imagine one connection staying in use for that long.
- 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.
Empty = no bytes; either way I think you can do without it by clean use
of the offset in the array of ByteBuffers.
Its internal so it doesn't matter so much.
$.02, Roger
Thanks.
--------------------------------------------------------------------------------
[1]https://github.com/reactive-streams/reactive-streams-jvm/#3-subscription-code
rule #17
