RFR JDK-8156742: Miscellaneous WebSocket API improvements
Simone Bordet
simone.bordet at gmail.com
Tue May 31 22:05:59 UTC 2016
Pavel,
On Tue, May 31, 2016 at 7:25 PM, Pavel Rappo <pavel.rappo at oracle.com> wrote:
> Hi,
>
> Could you please review the following change for JDK-8156742?
>
> http://cr.openjdk.java.net/~prappo/8156742/webrev.01/
Comments on the API only.
Looks much cleaner, good job.
I like WebSocket.request(long) being reinstated, and the Listener
methods taking a WebSocket as first parameter, Builder.header() being
sane :)
Further comments:
* 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.
* CloseReason
I would rename it to CloseInfo, as CloseReason hints to me of the
reason only, not the code.
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.
* 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 ?
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.
* 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.
Applications that wrap the default WebSocket object will have to implement it.
ws.sendText(stream.collect(joining())) is equivalent and as short.
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.
* Extensions
I don't recall if extensions have been ruled out ?
Browsers seem to have settled at implementing permessage-deflate.
Thanks !
--
Simone Bordet
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless. Victoria Livschitz
More information about the net-dev
mailing list