Hi Pavel
Thanks for your considerate response, replies are inline.
On 4/04/2016 13:08, Pavel Rappo wrote:
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.
Good point, in that case the current ordering is indeed clearer.
- 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?
Unlike with the sendText methods, I don't see the added value here. For
example, compare this with the method "header(String name, String
value)" in the Builder interface. To me this is the same: it might be
useful for callers if the parameters of that method were declared as a
CharSequence, but they aren't, and it would not be "Java-like"
(according to my totally subjective definition of that term) for them to
be either. Anyway, just my 2 cents.
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.
I'd argue that, since java.net.http will be a new package (& separate
module), its API should make as good use as possible of the available
types in the JDK. And in that regard, I believe a single Duration
parameter is clearer. And, though it doesn't apply in this case: an
advantage of Duration is that you can cleanly return it: Duration
getTimeout(), in which case it would be natural to have the setter
declared as void setTimeout(Duration t). While j.u.c.TimeUnit is indeed
more familiar, I don't see why this would speak in its advantage: it's
just a natural consequence from the fact that it's been around 10 years
longer than Duration already.
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).
Sorry, I hadn't gone through previous discussions. I assume the option
of returning Optional<CompletionStage<?>> has already been considered as
well then?
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?
Because I didn't think hard enough about it yesterday, I think :-)
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?
No, you're right: there shouldn't be any guarantees on the type (direct
or not) of the Buffer. However, about the copying: I'd count the current
decoding of the ByteBuffer into a CharBuffer as an unconditional copy as
well.
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?
Yes, that was a bad example, never mind.
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?
Agreed.
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!
Thanks to you,
Anthony
