Preliminary RFR JDK-8159053: Improve onPing/onClose behavior

[Pavel Rappo]

Hi Roger,

Thanks for having a look at this!

> On 22 Jun 2016, at 18:03, Roger Riggs <Roger.Riggs at oracle.com> wrote:
> 
> Hi Pavel,
> 
> in general, this treatment is fine, some comments below,
> 
>>  * <p> When a Close message has been received, the WebSocket Protocol
>>  * mandates it is handled in a certain way.
> 'a certain way' seems odd in a specification.  Instead, perhaps refer to the RFC as you have cited below,
> perhaps just move the reference up.
> 
> <a href="https://tools.ietf.org/html/rfc6455#section-5.5.1">Close</a>
> * and
> * <a href="https://tools.ietf.org/html/rfc6455#section-7.4">Status Codes</a>.

I agree.

>>  *
>>  * <p> After a Close message has been received, the {@code WebSocket}
>>  * will close automatically.However, the client may finish sending the
> "will be closed at the earliest of..."

I have had second thoughts on this paragraph. The thing is the current wording
precludes a use case which in my opinion we should allow. Namely, we should
allow the client to send their own Close message.

CompletionStage<?> onClose(WebSocket webSocket, int statusCode, String reason) {
    webSocket.sendClose(1000, "last_sent_id=...");
    return null;
}

As I understand the reason string was designed for things likes this, a final
piece of information an endpoint believes the other should have.

The spec is pretty vague in this area:

   ...If an endpoint receives a Close frame and did not previously send a
   Close frame, the endpoint MUST send a Close frame in response.  (When
   sending a Close frame in response, the endpoint typically echos the
   status code it received.)  It SHOULD do so as soon as practical.  An
   endpoint MAY delay sending a Close frame until its current message is
   sent (for instance, if the majority of a fragmented message is
   already sent, an endpoint MAY send the remaining fragments before
   sending a Close frame)...

To me it sounds like:

1. We should send the same code unless there's a reason not to (and that's what
the default implementation does)

2. We are allowed (naturally) to send any reason with it

Summing up, I would propose to change the whole paragraph to this:

* <p> After a Close message has been received, the {@code WebSocket}
* will close <i>automatically</i>. However, the client may finish
* sending the current sequence of message parts, if any. To facilitate
* this, the {@code WebSocket} will close after the completion of the
* returned {@code CompletionStage} or earlier if a Close message has
* been sent.

I think the behaviour of the WebSocket in case the client tries to start a new
messages sequence from within onClose (if the previous one has been finished) is
implementation specific. It might report to onError, or send a Close message
automatically or both. What's important is that the server SHOULD NOT receive
any part of the new message.

>>  * <p> The {@code statusCode} is an integer in the range {@code 1000 <= code
>>  * <= 4999}, except for {@code 1004}, {@code 1005}, {@code 1006} and {@code
>>  * 1015}*and some others *that can be only set by the {@code WebSocket}.
> "and some others" is too vague for a specification.  It should be possible to test every possible value
> as being allowed, indeterminate, or illegal.

Okay. Fair enough. The thing is the interpretation of the list of standard
status codes and their semantics is a bit opinionated. I tried to avoid rigidity
here. On the other hand maybe we could define what is 100% prohibited and 100%
acceptable? Like a black and a white lists. In this case we will have a bit more
freedom later.

What would you say about this?

* <p> The {@code statusCode} is an integer in the range {@code 1000 <= code
* <= 4999}.

...

* @implSpec The implementation is required to reject the following codes:
*
* <pre>
*     {@code 1004}, {@code 1005}, {@code 1006} and {@code 1015}.
* </pre>
*
* At the same time the following codes are accepted:
*
* <pre>
*     {@code 1000}, {@code 1001} and {@code 1008}.
* </pre>
*
* Accepting or rejecting other codes is implementation specific.
*
* @implNote This implementation rejects the following codes (in addition to
* those rejected by default):
*
* <pre>
*     {@code 1002}, {@code 1007}, {@code 1010} and {@code 1011}.
* </pre>
*

Thanks,
-Pavel