Hi,
On Fri, Oct 16, 2015 at 12:35 PM, Pavel Rappo <pavel.ra...@oracle.com> wrote:
> Hi,
>
> Here's a second update on the WebSocket API:
>
> webrev: http://cr.openjdk.java.net/~prappo/8087113/webrev.02/
> javadoc: http://cr.openjdk.java.net/~prappo/8087113/javadoc.02/
>
> Main differences from the previous version:
>
> * WebSocket has become an abstract class
What is the rationale behind this ?
Just to avoid subclassing ?
Why subclassing is negated ?
I think it's best it remains an interface: it would allow people to
write wrappers, use java.lang.reflect.Proxy, etc.
> * New methods in Builder: headers, connectTimeout
I still don't understand the headers() signature what benefits brings.
What problems would have a chainable: Builder header(name, value) ?
Clear semantic and no exceptions about an odd number of String passed in.
I am not sure that passing the listener to the Builder constructor is right.
There are applications that just stream in one sense, so there would
be no listener needed, so it appears strange that this is a required
constructor parameter rather than just another builder method.
> * WebSocket.Builder no longer accepts HttpRequest.Builder; only HttpClient
> * One Listener instead of many onXXX handlers
I tried to write a few examples with this API, and it's a bit cumbersome to use.
Below my feedback:
1. I think there is a mistake in onText(CharBuffer, boolean). Should
not be onText(CharSequence, boolean) ?
I don't think CharBuffer can handle properly UTF-8.
2. The split of the flow control functionality into a LongConsumer is
a little cumbersome to use.
This LongConsumer object needs to be carried around, typically in
conjunction with the WebSocket object, which forces applications to
pass around 2 parameters when one would have sufficed.
3. The absence of the WebSocket parameter from onXXX() methods makes
the API cumbersome to use.
It forces applications to write this boilerplate over and over:
new WebSocket.Builder("ws://localhost:8080/path", new WebSocket.Listener()
{
public WebSocket webSocket;
public LongConsumer controller;
@Override
public void onOpen(WebSocket webSocket, LongConsumer flowController)
{
this.webSocket = webSocket;
this.controller = flowController;
flowController.accept(1);
}
...
}
The WebSocket and LongConsumer needs to be saved from onOpen() to be
used in other methods, and it's boilerplate code that needs to be
written all the time.
I suggest to consider to modify the signature of the onXXX() methods to:
CompletableFuture<?> onText(WebSocket, CharSequence, boolean)
and to merge again LongConsumer back into WebSocket. That would make
the API soo much easier to use.
4. WebSocket.Listener should have all methods implemented with a
default implementation. It's just too much to have to implement them
all when all I want is to receive text messages.
If WebSocket is passed as parameter to onXXX() methods and
LongConsumer is merged back into WebSocket, it would be trivial to
write a correct default implementation of the Listener interface
(which is now impossible).
> * Completion handlers with custom contexts are gone;
Finally :)
> * send methods return CompletableFuture<WebSocket>
This parameter is typically not used.
When an application calls sendXXX() it already has the WebSocket
reference in scope, so it would be available anyway.
Furthermore, the removal of the flow control functionality from
WebSocket makes the code a little weird (a simple echo below):
CompletableFuture<?> onBinary(ByteBuffer payload, boolean isLast)
{
return this.ws1.sendBinary(payload, isLast).thenAccept(ws2 ->
this.flowController.accept(1));
}
As you can see there are two "ws" references when one would be enough,
and the lambda passed to thenAccept() takes "ws2" that is useless,
because the application needs the flow controller, not the WebSocket.
And that flow controller is associated to "ws1" (from onOpen()), so
having to deal with ws2 is confusing (is it the same reference ? it's
something else ?)
I think that re-merging the flow controller functionality in WebSocket
and passing WebSocket as first parameter to onXXX() methods will
simplify a lot, and make possible to write completely stateless
listeners that would come handy when you are opening a large number of
connections (you can only use one instance, rather than one per
connection).
Method onOpen() still has reason to exist, of course.
> * onXXX methods in Listener return CompletableFuture<?> as a means of
> asynchronous completion signalling
The downside of using CF is the allocation of CF instances that the
application is forced to make to comply with the API.
Given that this is a low-level API, I have a preference for using
Consumer<Throwable> in all cases.
> * StatusCode is now ClosureCode
"Closure" typically means something different to a programmer.
I would rename to CloseCode or similar.
Given that exceptions are relayed via methods (and not try/catch), I
find no use for WebSocketException subclasses.
There may be a need for WebSocketException in case of protocol errors
or spec violations, but I don't see how
WebSocketException.AsynchronousClose or WebSocketException.Handshake
are any useful.
WebSocketException.AsynchronousClose seems a duplication of
java.io.AsynchronousCloseException, and WebSocketException.Handshake
can just be relayed as a WebSocketException.
I would rather drop the inner classes and just keep WebSocketException only.
The subclasses also break an "implicit" naming convention where
exceptions class names always have the "Exception" suffix. The JDK
consistently use this naming convention apart for some 1.0 class for
historical reasons (e.g. ThreadDeath).
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
