RFR [9] 8180155: WebSocket secure connection get stuck after onOpen

[Daniel Fuchs]

Hi Pavel,

On 01/06/2017 12:32, Pavel Rappo wrote:
> Hi Daniel,
> 
> Thanks a lot for looking into this! I agree there's an issue exactly where you
> pointed at. I'm sure it would be a good idea to write a test that covers this.
> Unfortunately it would be quite a challenging task.

I agree it would be good to have a test :-(
These kind of complex state logic have a tendency to
bite you if you only test them by code reading.

> I tried to rewrite the state machine the way that both fixes this bug and makes
> the code easier to follow (I'm biased here obviously).

Can we throw an InternalError or assert if
124                         reader.readFrame(data, frameConsumer);
does not advance the buffer position?

131                 break;
hmmm... what if demand.get() is no longer 0 when we reach this line?
what if the cooperative handler already thinks that the current thread
will read the data? Is that possible?

best regards,

-- daniel


> Please have a look at the updated webrev and tell me if you're generally happy
> with this approach:
> 
>      http://cr.openjdk.java.net/~prappo/8180155/webrev.01/
> 
> So here are a couple of points to keep in mind while reasoning about the code:
> 
>    1. There's at most one channel readiness event registered at any given time
>    2. There's at most one thread executing `pushContinuously` at any given time
>       (enforced by CooperativeHandler)
> 
> Thanks!
> 
>> On 31 May 2017, at 12:13, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> Hi Pavel,
>>
>> Receiver.java:
>>
>> 120     private void pushContinuously() {
>> 121         while (true) {
>> 122             if (!readable.get()) {
>> 123                 if (!initialized) {
>> 124                     initialized = true;
>> 125                     try {
>> 126                         channel.registerEvent(event);
>> 127                     } catch (IOException e) {
>> 128                         messageConsumer.onError(e);
>> 129                     }
>> 130                 }
>> 131                 break;
>> 132             } else if (demand.get() > 0 && !handler.isStopped()) {
>> 133                 pushOnce();
>> 134             } else {
>> 135                 break;
>> 136             }
>> 137         }
>> 138     }
>>
>> I think you might have an issue here, if the initialBuffer
>> already contains all the data that there is to be read, then
>> the readable.get() will be false, initialized will be false,
>> pushOnce() will not be called, and you're going to register
>> an event that will never be triggered.
>>
>> best regards,
>>
>> -- daniel
>>
>> On 31/05/2017 11:30, Pavel Rappo wrote:
>>> Hello,
>>> Please review the following change:
>>>   http://cr.openjdk.java.net/~prappo/8180155/webrev.00/
>>> This change addresses 2 separate issues:
>>>      https://bugs.openjdk.java.net/browse/JDK-8180155
>>>      https://bugs.openjdk.java.net/browse/JDK-8156518
>>> The first one is a busy-wait for data on the socket which manifests itself as
>>> non-returning invocation of WebSocket.request(long).
>>> The second one is an issue with a timeout for an asynchronous HTTP request.
>>> This issue affects both HttpClient and WebSocket implementations.
>>> Thanks,
>>> -Pavel
>>
>