> On 16 Oct 2015, at 22:11, Simone Bordet <simone.bor...@gmail.com> wrote:
>
> 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.
This argument can be used for pretty much any class out there. You see, it's a
question of whether subtyping of X should be allowed or disabled, if X has not
been designed with it in mind.
>> * 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.
The reason was that other Builder's methods (now only 2) have a "replace"
semantics rather than "add":
* <p> If a particular intermediate method is not called, an appropriate
* default value (or behavior) is used. A repeated call to an intermediate
* method overwrites the previous value (or overrides the previous
* behaviour), given no exception is thrown.
If Builder.header was with "add" semantics, it wouldn't be possible to remove
added headers. But now I probably agree with you. In this case it would be
better for the API to provide an "add" behaviour, mostly because it's more
compile time checking friendly.
> 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.
Maybe you're right. For now let's remember this argument as (A) to return to it
later.
>> * 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.
I'm not sure I understand what you mean by "handle properly UTF-8". First of
all, CharBuffer implements CharSequence. Secondly, if a user decides to create a
ByteBuffer out of a Text message, CharBuffer is the way to go.
It's a native class for charset decoding/encoding:
java.nio.charset.CharsetEncoder#encode(java.nio.CharBuffer).
No additional conversions are needed. On the other hand, if the user needs a
String, then simple CharBuffer.toString would do. In my opinion, using
CharBuffer directly is the best of two worlds.
> 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.
Easier? In some cases, probably. On the other WebSocket.request(long) is an
internal part of the API that only the Listener should talk to (the same is with
Flow.Subscriber and Flow.Subscription). Moreover, it would be strange (remember
argument (A)?) that one could request something not having any means of
listening to it.
Do we have any good reasons to merge flowController with WebSocket other than
boiler-plate eliminating? Because we just might lose good level of separation
of concerns.
> 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).
Summing up (A) and the passage above, I would provide some AbstractListener that
would take care of a boilerplate code.
>> * Completion handlers with custom contexts are gone;
>
> Finally :)
>
>> * send methods return CompletableFuture<WebSocket>
>
> This parameter is typically not used.
It's too early to refer to something in this API as "typically" :-)
Or you mean for CF in general?
> When an application calls sendXXX() it already has the WebSocket
> reference in scope, so it would be available anyway.
You're right.
> 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).
Good argument! We may try to accomplish it a bit differently. But I'm against of
merging flowController with WebSocket.
> 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.
In terms of composability those approaches are pretty much the same:
void onText(CharBuffer text, boolean isLast, Consumer<Throwable> handler) {
Consumer<Throwable> throwableConsumer = (t) -> {
if (t == null) flowController.accept(1);
};
sendText(text, isLast, throwableConsumer.andThen(handler::accept));
}
or
CompletableFuture<?> onText(CharBuffer text, boolean isLast) {
return ws.sendText(text, isLast).thenRun(() -> flowController.accept(1));
}
The difference might become serious if we think about one-shot vs reusable
objects. In general. Not about CF vs handler. Reusable handlers are more prone
to bugs as far as I can see now. Because they are inherently same objects but
being used in different time (context). Thus their .accept(Throwable) would mean
different thing in different time. Hence more caution would be needed,
especially
on the receiving side, when the app gets an implementation's handler.
Implementation's code will have to become a lot more sophisticated in this case,
because it will now have to correlate a handler to an invocation. (I know nobody
cares about the implementation. I'm just saying.)
As for the API, well CompletableFuture is surely more nice compared to a bare
Consumer. But let me try to implement both things and we could choose later.
We probably don't want to double the number of sending methods, do we? :)
>> * StatusCode is now ClosureCode
>
> "Closure" typically means something different to a programmer.
> I would rename to CloseCode or similar.
I dunno. RFC 6455 uses "closure" all over the the place, e.g.:
As defined in Sections 5.5.1 and 7.4, a Close control frame may
contain a status code indicating a reason for closure.
I think we should ask for an opinion from others. But hey, it's not a
com.sun.tools.javac package we're designing a WebSocket for. I'm sure there
won't be any ambiguity.
> Given that exceptions are relayed via methods (and not try/catch), I
> find no use for WebSocketException subclasses.
What exclusive things does try-catch provide we couldn't emulate with good old
instanceof/getClass()? Well, maybe just a tiny bit of help from a compiler. Or
you say with method-relayed exceptions there's no need to know their type at
all?
> 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.
Handshake provides a java.net.http.HttpResponse (Joakim [*] has noticed it might
be important). And you say you don't see how a user would benefit from knowing
some of their send operations have been terminated due to asynchronous close?
> WebSocketException.AsynchronousClose seems a duplication of
> java.io.AsynchronousCloseException, and WebSocketException.Handshake
> can just be relayed as a WebSocketException.
You mean java.nio.channels.AsynchronousCloseException? Well yes, but it's not
reusable. The wording and the package confine this exception to channels and
buffers.
> 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).
Good argument. Will return to it later. Thanks!
> 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
-------------------------------------------------------------------------------
[*] http://mail.openjdk.java.net/pipermail/net-dev/2015-August/009078.html
