Hi Roger, Thanks for having a look at this!
> On 22 Jun 2016, at 18:03, Roger Riggs <roger.ri...@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
