Preliminary RFR JDK-8159053: Improve onPing/onClose behavior

[Simone Bordet]

Hi,

On Wed, Jul 13, 2016 at 3:05 PM, Pavel Rappo <pavel.rappo at oracle.com> wrote:
>     /**
>      * Sends a Close message with the given status code and the reason.
>      *
>         ...
>      *
>      * <p> The {@code statusCode} is an integer in the range {@code 1000 <= code
>      * <= 4999}. However, not all status codes may be legal in some
>      * implementations. {@code 1000} (Normal Closure) is always legal and {@code
>      * 1002}, {@code 1003}, {@code 1005}, {@code 1006}, {@code 1007}, {@code
>      * 1009}, {@code 1010}, {@code 1012}, {@code 1013} and {@code 1015} are are
>      * always illegal codes.
>      *
>      * <p> The {@code reason} is a short string that must have an UTF-8
>      * representation not longer than {@code 123} bytes. For more details on
>      * Close message, status codes and reason see RFC 6455 sections
>      * <a href="https://tools.ietf.org/html/rfc6455#section-5.5.1">5.5.1. Close</a>
>      * and
>      * <a href="https://tools.ietf.org/html/rfc6455#section-7.4">7.4. Status Codes</a>.
>      *
>         ...
>      *
>      * @throws IllegalArgumentException
>      *         if the {@code statusCode} has an illegal value, or
>      *         {@code reason} doesn't have an UTF-8 representation not longer
>      *         than {@code 123} bytes

Too many negations on the reason.
Consider "or {@code reason} has a UTF-8 representation longer than 123 bytes".

>      *
>      * @see #sendClose()
>      */
>     CompletableFuture<WebSocket> sendClose(int statusCode, String reason);
>
> Oh dear, I know Simone won't probably like it, but please bear with me while I'm
> trying to explain the rationale behind all this.

I understand the rationale behind forbidding certain codes, and I'm ok with it.

What I don't like is that illegal status codes are signaled by
throwing, rather than failing the CF.
You did not explain *that* rationale :)

Furthermore, it's not clear in what status is the WS being left after the throw.
Is it closed with a different code ?
Should the application try to close again ?

In general, I think you need to decide what to do with throwing exceptions.
I like just one mechanism.
It is always that one mechanism, everybody knows it, no clunky
additional try/catch blocks.

Consider the case of a too long reason.
The current specification forces to perform the conversion to UTF-8
bytes immediately because if it's too long, the implementation must
throw.

On the other hand, if the failure is signaled via the CF, the
implementation can delay the conversion until really necessary and, if
it fails, notify the CF.
Possibly, never perform the conversion because the WS has already been
otherwise closed.

Consider also:

ws.sendClose(1000, too_long).handle((ws, th) -> cleanup());

versus:

try {
  ws.sendClose(1000, too_long).handle((ws, th) -> cleanup());
} catch (Throwable x) {
  // Uh, no idea if the implementation actually closed !
  // Let's *hope* this one goes fine :/
  ws.sendClose(1000, "short").handle((ws, th) -> cleanup());
}

-- 
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