RFR JDK-8087113: Websocket API and implementation

[Pavel Rappo]

> On 29 Mar 2016, at 20:16, Andrej Golovnin <andrej.golovnin at gmail.com> wrote:
> 
> JEP-280 does not apply in this case. But when you rewrite this code to
> use the plain String concatenation, then JEP-280 would apply:
> 
> 211     static String toString(Buffer b) {
> 212         return toStringSimple(b)
> 214                + "[pos=“ + b.position()
> 215                + " lim=“ + b.limit()
> 216                + " cap=“ + b.capacity()
> 217                + " rem=“ + b.remaining() + "]";
> 218     }
> 
> This code is easier to read than the one with StringBuilder, at least for me.

I will rewrite this (and similar) code using "+".

>>> src/java.httpclient/share/classes/java/net/http/SignalHandler.java
>>> 
>>> Maybe the lines 33-46 should be converted to a JavaDocs comment.
>>> 
>>> 112             switch (s) {
>>> 113             case RUNNING:
>>> 114                 return RERUN;
>>> 115             case DONE:
>>> 116                 return RUNNING;
>>> 117             case RERUN:
>>> 118                 return RERUN;
>>> 119             default:
>>> 120                 throw new InternalError(String.valueOf(s));
>>> 121             }
>>> 
>>> Please fix the indentation.
>> 
>> I've tried to adhere to this:
>> 
>> 	http://cr.openjdk.java.net/~alundblad/styleguide/#toc-indentation
>> 
>> Might revisit later. I will wait for the status change of this document.
> 
> Btw. have you considered to use an array instead of switch-statement?
> The array may look like this:
> 
> private static final int[] NEXT_STATE = new int[] {
>    RUNNING, // DONE -> RUNNING
>    RERUN,     // RUNNING -> RERUN
>    RERUN,     // RERUN -> RERUN
> };
> 
> And then the line 111 can be written as:
> 
> 111         int prev = state.getAndUpdate(s -> NEXT_STATE[s]);
> 
> The only advantage is that it produces less byte codes.
> But maybe it is still worth.

I don't think it would be as readable as switch-case is.

BTW, it looks like there's a newer (Draft, v6) version of the document
(accessible through the same link), which fixes the switch-case formatting
style. Quite a volatile document, indeed :-)

I will fix the indentation.

> I have already started to port our code to use the new API and I have one comment.
> I think you should introduce a new class CloseReason as it was done in JSR 356
> (http://docs.oracle.com/javaee/7/api/javax/websocket/CloseReason.html)
> and use it in #sendClose and #onClose-methods. The #onClose-method in the
> WebSocket.Listener interface looks really ugly:
> 
> onClose(WebSocket webSocket, Optional<WebSocket.CloseCode> code, String reason)
> 
> The parameter “reason” makes only sense when code.isPresent() returns true.
> The code and the reason belong together. But the one is wrapped in Optional and the other not.
> It doesn’t feel right. But when you introduce the new class CloseReason, then
> you can change this method to:
> 
> onClose(WebSocket webSocket, Optional<WebSocket.CloseReason> closeReason)
> 
> I think it makes the API much better and it is clear that the reason phrase is only
> available when you have a close code too.

Very true. The API looks awkward in this particular place. On the other hand, it
would look a bit of an overkill for the close type of message to define 2 types
(3 types in JSR 356).

Maybe we could somehow melt what you call CloseReason and CloseCode into a
single entity? Something like (different name is an option too):

    public final class CloseReason {
    
        static CloseReason normalClosure(CharSequence reason);
        static CloseReason goingAway(CharSequence reason);
        // ...
        static CloseReason unexpectedCondition(CharSequence reason);
        
        static CloseReason of(int code);
        static CloseReason of(int code, CharSequence reason);
    }

In an application:

    ws.sendClose(CloseReason.normalClosure("Too tired"));
    ws.sendClose(CloseReason.of(2016, "Leap year"));

This doesn't solve the first problem of not having code description, but seems
to solve the second one.

> And I think there should be a way to configure a timeout for sending messages.

As I understand timeout expiration for sendXXX would be a fatal condition (in
general.) Simply because by the time a returned CF has completed exceptionally,
some bytes of a message might have been sent already. (That's generally not the
case with read timeouts see java.net.Socket#setSoTimeout)

What would you expect _additionally_ from the implementation (API guarantees) in
a case where send completes exceptionally with a timeout in contrast with this
snippet?

    ws.sendBinary(giganticByteBuffer, true)
            .orTimeout(30, TimeUnit.SECONDS)
            .whenComplete((v, e) -> {
                if (e instanceof TimeoutException) {
                    try {
                        ws.abort();
                    } catch (IOException ignored) { }
                }
            });

BTW, I'm thinking of changing sendXXX methods return type to CompletionStage
(now it's CompletableFuture), this will cause, for example,
CompletableFuture#get(long, java.util.concurrent.TimeUnit) not to be directly
accessible.

> One minor thing off-topic:
> While changing my code I noticed that the class ProxySelector$StaticProxySelector
> and the JavaDocs of the ProxySelector#of-method can be improved a little bit:
> 
> diff -r 589795e4cd38 src/java.base/share/classes/java/net/ProxySelector.java
> --- a/src/java.base/share/classes/java/net/ProxySelector.java	Wed Mar 23 19:33:39 2016 -0700
> +++ b/src/java.base/share/classes/java/net/ProxySelector.java	Mon Mar 28 19:30:24 2016 +0200
> @@ -165,7 +165,8 @@
> 
>     /**
>      * Returns a ProxySelector which uses the given proxy address for all HTTP
> -     * and HTTPS requests. If proxy is {@code null} then proxying is disabled.
> +     * and HTTPS requests. If the given proxy address is {@code null}
> +     * then proxying is disabled.
>      *
>      * @param proxyAddress
>      *        The address of the proxy
> @@ -178,9 +179,9 @@
>         return new StaticProxySelector(proxyAddress);
>     }
> 
> -    static class StaticProxySelector extends ProxySelector {
> +    private static final class StaticProxySelector extends ProxySelector {
>         private static final List<Proxy> NO_PROXY_LIST = List.of(Proxy.NO_PROXY);
> -        final List<Proxy> list;
> +        private final List<Proxy> list;
> 
>         StaticProxySelector(InetSocketAddress address){
>             Proxy p;
> @@ -198,9 +199,9 @@
>         }
> 
>         @Override
> -        public synchronized List<Proxy> select(URI uri) {
> -            String scheme = uri.getScheme().toLowerCase();
> -            if (scheme.equals("http") || scheme.equals("https")) {
> +        public List<Proxy> select(URI uri) {
> +            String scheme = uri.getScheme();
> +            if (scheme.equalsIgnoreCase("http") || scheme.equalsIgnoreCase("https")) {
>                 return list;
>             } else {
>                 return NO_PROXY_LIST;

CC'ing Michael.