Hi Anthony, thanks a lot for looking into this!
> On 3 Apr 2016, at 17:45, Anthony Vanelverdinghe
> <anthony.vanelverdin...@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.
> - 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?
> 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.
> 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).
> 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?
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?
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?
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?
> 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!
