RFR JDK-8087113: Websocket API and implementation

Wed Apr 6 20:28:54 UTC 2016

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.Riggs at 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
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160406/3a69f2e0/attachment-0001.html>