RFR JDK-8087113: Websocket API and implementation

[Anthony Vanelverdinghe]

Hi Pavel

Thanks for your considerate response, replies are inline.

On 4/04/2016 13:08, Pavel Rappo wrote:
> Hi Anthony, thanks a lot for looking into this!
>
>> On 3 Apr 2016, at 17:45, Anthony Vanelverdinghe <anthony.vanelverdinghe at gmail.com> wrote:
>>
>> Here are my suggestions concerning the public types:
>>
>> java.net.http.WebSocket
>> - order the arguments of
>> static Builder newBuilder(URI uri, HttpClient client, Listener listener)
>> consistently with the 2-argument method:
>> static Builder newBuilder(URI uri, Listener listener, HttpClient client)
> I see your concern. I've considered  using the same signature. But I chose the
> other one for a reason. There are pros and cons for both versions, of course.
> If we choose the proper telescoping version
>
>     newBuilder(URI uri, Listener listener, HttpClient client)
>
> then it would be more consistent with the 2-arg method indeed. On the other hand
> it would make a use of an in-place created Listener a bit more ugly. Imagine a
> huge anonymous "listener" in the middle with an almost unnoticeable tiny
> "client" in the end of the argument list:
>
>      WebSocket.newBuilder(URI.create("ws://myws"), new Listener() {
>
>          @Override
>          public void onOpen(WebSocket webSocket) {
>          ...
>          }
>
>          ...
>
>          @Override
>          public CompletionStage<?> onPong(WebSocket webSocket, ByteBuffer message) {
>          ...
>          }
>      }, client);
>
> I would prefer to have something extensible moved to the very end of the list of
> arguments. But that's just my opinion.
Good point, in that case the current ordering is indeed clearer.
>> - remove CompletableFuture<Void> sendText(Stream<? extends CharSequence> message):
>> there are many candidate convenience methods, and the choice for Stream<? extends CharSequence> seems arbitrary. For example, why this one, and not methods with char[] (analog to the byte[] method for sendBinary), Iterable<? extends CharSequence> (as used by java.nio.file.Files::write), Reader, ...
> java.util.stream.Stream would be a more general source than java.lang.Iterable.
>
>> convenience methods can always be added in a later version, based on empirical evidence of which convenience methods would add most value
> Absolutely!
>
>> - remove CompletableFuture<Void> sendBinary(byte[] message, boolean isLast):
>> same motivation as the previous one
>>
>> - add CompletableFuture<Void> sendBinary(ByteBuffer message)
>> to be consistent with sendText, which has the single-parameter method sendText(CharSequence message)
> I will think about it.
>
>> - CompletableFuture<Void> sendClose(CloseCode code, CharSequence reason)
>> change the type of "reason" to String
> What's the rationale behind this?
Unlike with the sendText methods, I don't see the added value here. For 
example, compare this with the method "header(String name, String 
value)" in the Builder interface. To me this is the same: it might be 
useful for callers if the parameters of that method were declared as a 
CharSequence, but they aren't, and it would not be "Java-like" 
(according to my totally subjective definition of that term) for them to 
be either. Anyway, just my 2 cents.
>> java.net.http.WebSocket.Builder
>> - Builder connectTimeout(long timeout, TimeUnit unit)
>> use a single java.time.Duration argument instead
> I'm not too familiar with the new java.time API, but maybe you could explain
> what would some advantages be over the java.util.concurrent one?
>
> The one I can think of immediately is the encapsulation and consistency check
> would be done by java.time rather than by java.net.http.WebSocket. On the other
> hand java.util.concurrent.TimeUnit is more familiar.
I'd argue that, since java.net.http will be a new package (& separate 
module), its API should make as good use as possible of the available 
types in the JDK. And in that regard, I believe a single Duration 
parameter is clearer. And, though it doesn't apply in this case: an 
advantage of Duration is that you can cleanly return it: Duration 
getTimeout(), in which case it would be natural to have the setter 
declared as void setTimeout(Duration t). While j.u.c.TimeUnit is indeed 
more familiar, I don't see why this would speak in its advantage: it's 
just a natural consequence from the fact that it's been around 10 years 
longer than Duration already.
>> java.net.http.WebSocket.Listener
>> - onText/onBinary/onPing/onPong
>> return an immediately-complete CompletionStage (i.e. CompletableFuture.completedStage(null)) instead of null
> We've been through this. It's a matter of both performance AND convenience. A
> user gets both convenience of returning null and reduction of allocation.
> Moreover if we go with disallowing null, people might be tempted to use a single
> shared instance
>
>     CompletableFuture.completedStage(null)
>
> as a return value. I'd better stay away from it (though it seems technically
> possible, it still feels hacky).
Sorry, I hadn't gone through previous discussions. I assume the option 
of returning Optional<CompletionStage<?>> has already been considered as 
well then?
>> return CompletionStage<Void> instead of CompletionStage<?>
> I thought about this and discarded previously. The reason is simple. It would
> kill the simple composability, when one could easily return a CS<what-have-you>
> from any of the onXXX methods. Because CS<what-have-you> can always be assigned
> to CS<?>. Once again, it's a convenience to the user.
>
>> - onText
>> change the type of the "message" parameter with ByteBuffer, and specify it contains UTF-8 encoded bytes & has a backing array
>>
>> java.net.http.WebSocket.Text
>> remove this interface. I believe the Text interface doesn't carry its weight. By specifying the Listener::onText method as above, one can easily do something like new String().getBytes(message.array(), UTF_8) to get a CharSequence
> Interesting. You're willing to give away this thing. May I ask why?
Because I didn't think hard enough about it yesterday, I think :-)
> First of all, in today's realities backing array would simply mean that the
> ByteBuffer is not direct, and since data from the SocketChannel is read into
> direct buffers _internally_ , it would mean that you've got a copy. Would you
> like your data to be unconditionally copied every time?
No, you're right: there shouldn't be any guarantees on the type (direct 
or not) of the Buffer. However, about the copying: I'd count the current 
decoding of the ByteBuffer into a CharBuffer as an unconditional copy as 
well.
> Secondly, WebSocket Text messages may arrive in fragments, which guaranteed to
> be proper UTF-8 sequences only after fully assembled. So a fragment may arrive
> as an _incomplete_ UTF-8 sequence and the
>
>     new String().getBytes(message.array(), UTF_8)
>
> wouldn't do. So if we remove this, then it means each and every user will have
> to deal with all sorts of lovely character issues by themselves. Now, as an
> implementor, I would be more than happy to unload this burden from myself :-) To
> be honest that's the most troublesome piece of code so far. But what about
> users?
Yes, that was a bad example, never mind.
> If you want the implementation to reshape the buffers so that they contain only
> proper sequences, then the implementation will have to parse & buffer the
> incoming stream. Given that it already does this (parsing) for the sake of
> verification required by the spec, wouldn't it be nice to provide characters as
> a by-product so the user won't have to bother?
Agreed.
>> java.net.http.WebSocketHandshakeException
>> - extend IOException, consistently with all other exception classes in java.net and javax.net
> I will think about it.
>
>> - if HttpResponse can indeed be null (as is documented in getResponse), the constructor's current implementation will throw a NullPointerException in this case
> The reason is simple: Serialization. All exceptions must be serializable. But
> that's just a corner case. I will add the comment to the javadoc as was
> suggested previously by Joe Darcy:
>
>      /**
>       * Returns a HTTP response from the server.
>       *
> ->   * <p> The value may be unavailable ({@code null}) if this exception has
> ->   * been serialized and then read back in.
>       *
>       * @return server response
>       */
>
> Thanks!
>
Thanks to you,
Anthony