RFR [9] 8179021: Latest bugfixes to WebSocket/HPACK from the sandbox repo

Daniel Fuchs daniel.fuchs at oracle.com
Wed May 10 12:31:34 UTC 2017


Hi Pavel,

Looks good.
Some files didn't have their copyright years updated - maybe
you could fix that before pushing.

jdk/incubator/http/internal/websocket/OpeningHandshake.java
line 121: It might be worth mentioning that '13' is mandated by
[RFC 6455](https://tools.ietf.org/html/rfc6455)

best regards,

-- daniel

On 10/05/2017 12:03, Pavel Rappo wrote:
> Daniel, thanks a lot for looking into this! I've addressed most of your suggestions:
>
>     http://cr.openjdk.java.net/~prappo/8179021/webrev.01/
>
>> On 9 May 2017, at 15:49, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> Hi Pavel,
>>
>> A few nits:
>>
>> jdk/incubator/http/WebSocket.java:
>>
>> line 501: should use {@linkplain
>>
>> Also (line 46 and 501) the link target #sendClose may cause trouble
>> if sendClose is overloaded - so I wonder if it may be better to
>> have the  full prototype there:
>>   #sendClose(int, String)
>> (though I guess it's OK as long as there is only 1 sendClose method).
>>
>> jdk/incubator/http/internal/websocket/OutgoingMessage.java
>>
>> 75         // maskingKeys, CharsetDecoder and should be here
>>
>> what does this means? are some words missing?
>>
>> jdk/incubator/http/internal/websocket/Transmitter.java
>>
>>  58      * The supplied handler may be invoked in the calling thread, so watch out
>>  59      * for stack overflow, if there's a possibility of send being called again
>>  60      * from the handler.
>>
>> Can we reformulate this again?
>> Something like:
>>
>>        * The supplied handler may be invoked in the calling thread.
>>        * A {@code StackOverflowException} may thus occur if there's a
>>        * possibility that this method is called again by the supplied
>>        * handler.
>>
>>  93                   // or tuned off
>>
>> I guess you meant 'turned off'
>>
>> jdk/incubator/http/internal/websocket/WebSocketImpl.java
>>
>> 196         if (alreadyCompleted) {
>> 197             throw new InternalError();
>> ...
>> 321         if (!added) {
>> 322             throw new InternalError();
>>
>> A comment similar to the other places where InternalError is thrown
>> would be nice.
>>
>> best regards,
>>
>> -- daniel
>>
>>
>> On 09/05/2017 14:52, Pavel Rappo wrote:
>>> Hello,
>>>
>>> Please review the following change:
>>>
>>>  http://cr.openjdk.java.net/~prappo/8179021/webrev.00/
>>>
>>> Along with refactoring this change contains a number of critical fixes for
>>> WebSocket. Critical fixes are applied to Receiver, WebSocketImpl and
>>> OpeningHandshake.
>>>
>>> Also this change removes 2 convenience methods and 1 constant from WebSocket
>>> interface. Since this interface is a part of incubation feature there shouldn't
>>> be any problem. Those members were initially controversial.
>>>
>>> Thanks,
>>> -Pavel
>>>
>>
>



More information about the net-dev mailing list