RFR JDK-8156742: Miscellaneous WebSocket API improvements

[Pavel Rappo]

Hi Simone, many thanks for taking a close look at this!

> On 31 May 2016, at 23:05, Simone Bordet <simone.bordet at gmail.com> wrote:
> 
> * What is interface Text for ?
> If it does perform a bytes-to-chars conversion, then offering
> asByteBuffer() can be easily done by the application.
> A websocket implementation must check that the incoming text bytes are
> indeed UTF-8. Doing the check is equivalent to creating the
> corresponding String, so I'm not sure Text is of any help.

This interface was scheduled for removal [1] some time ago. I hope to address it
in the next couple of weeks.

> * CloseReason
> I would rename it to CloseInfo, as CloseReason hints to me of the
> reason only, not the code.

Good point. But only for people who are closely familiar with the RFC 6455
and/or feel overly attached to it ;)

Believe me, I thought a lot about the name we could give to a single type
representing the contents of a Close message. One of the ideas that helped me to
make a decision was that JSR 356 already uses this name [2] in the similar
context.

If your opinion is strong, then please let's bikeshed on this issue later.
Are you ok with it?

> I think this class exposes too many failure codes that applications
> *must not* be able to use.
> For example, 1002 is not something that the application can ever send,
> only the implementation can, and having a public API to create a 1002
> CloseInfo is not something you I'd like to see exposed.
> Same goes with 1007, which the implementation must detect, not the
> application; etc.
> I would probably just leave CloseInfo.of(), with the current checks
> you are doing extended.

Thinking...

> * onClose() semantic.
> I am not sure why CloseInfo is wrapped with an Optional ?
> Can't the implementation just synthesize a (1005, "") instead and get
> rid of the Optional ?

Yes, we _can_ represent an empty Close message as (1005, ""). That's what this
status code was designed to do. But do we _have to_? I think it's totally up to
an implementation. 

If it decided to do so, it should leave an appropriate @implNote.

> Also, I think it should return a CF, for the following reason.
> Notification of onClose() is a half-close. Applications are entitled
> to send data from within onClose().
> For such reason, the implementation cannot send the response close
> frame just after the method returns.
> It should wait until the application has finished writing, and hence
> the need for the CF.

I hear you. I think that's where our understanding diverge. Please bear with me
while I'm trying to explain this. Originally the idea was that the API should be
a low-level API (that's btw why we have lots of these status codes exposed).

So the user could intercept/override standard actions and do whatever they like.
Consider `Listener.onPing` for example. By default it sends a ping as a
response. But if overridden it might not do that. Instead it will do whatever is
coded in the `Listener.onPing` method. Once again. It doesn't not send the Pong
message in reply to received Ping _unconditionally_ when either CS completes or
the method returns.

The same thing with `Listener.onClose`. Right now it does nothing. It's up to
the user to fully define the semantics of it. Because only user knows when they
are ready to send a Close message (consider a situation when a long
message/series of message have been being sent, then the user might decide to
send a Close message upon completion).

Now. I think you misunderstood the current state of API. But your understanding
in fact may be a lot more useful. I can see how the user would return an
incomplete CS from `Listener.onClose`, saying "Hey, close when this is done,
thanks." Interesting.

Simone, may I ask you what you think about current `Listener.onPong()`
behaviour? Do you think it should also send Pong unconditionally upon completion
of returned CS?

> * sendText(Stream<? extends CharSequence> message);
> I think it's too much for a utility method. It's a rare use case, I
> don't think it's worth it.

Pretty much everything these days can be seen as streams. Text files from
`java.io.BufferedReader#lines`, providing streams from a collection, creating
own streams, etc. Ah, yes. A Stream can be infinite or at least much, much
bigger than what one is wheeling to sacrifice in the heap memory. Basically the
idea was that if it's worth adding a good high-level method, this should be the
one.

> Applications that wrap the default WebSocket object will have to implement it.
> ws.sendText(stream.collect(joining())) is equivalent and as short.

If I'm not mistaken this is not an equivalent. First of all I don't think it
would be an expected behaviour to send an empty Text message if the stream is
empty.

I think there's a difference between an empty stream of strings and a stream of
empty strings.

We can argue about it, of course. For example, 

    java.nio.file.Files#write(Path, Iterable<CharSequence>, Charset, OpenOption...)
    
Will create the file at path if Iterable doesn't have the next element (empty).

Secondly, what about huge (or even infinite) streams? Do you really want them to
be collected in a String?

> If the goal was to send one frame per string, there is almost zero
> chance that the exact fragmentation is maintained at the server, so
> once again I don't see the reason for this method.

No, it was not a goal.

> * Extensions
> I don't recall if extensions have been ruled out ?
> Browsers seem to have settled at implementing permessage-deflate.


We decided to skip it for now, and address later [3]. As for permessage-deflate,
as I understand it can be provided in the minor update of JDK 9 with some sort
of System.property toggle or even an impl. default behaviour (if badly needed).

Thanks,
-Pavel

--------------------------------------------------------------------------------
[1] https://bugs.openjdk.java.net/browse/JDK-8156650
[2] https://docs.oracle.com/javaee/7/api/javax/websocket/Session.html#close-javax.websocket.CloseReason-
[3] https://bugs.openjdk.java.net/browse/JDK-8138949