Code Review Request: 8028562

[Xuelei Fan]

I just noticed this. "read error" does not describe the issue properly.
 The underlying issue is that client message may be fragmented into
small pieces during the TCP transaction. It's reasonable although it is
pretty hard to reproduce.

How about:
- // will try to read one more time if there is a read error
+ // will try to read one more time in case client message
+ // is fragmented to multiple pieces

Xuelei

On 12/6/2013 3:45 PM, zaiyao liu wrote:
> Hi Sean,
> 
> Thanks for your suggestion, updated please review:
> http://cr.openjdk.java.net/~ewang/kevin/JDK-8028562/webrev.00/
> 
> Thanks again,
> 
> Kevin
> On 2013/12/6 1:13, Sean Mullan wrote:
>> Sorry for the late comment, but there is a typo in this comment (roud
>> -> round):
>>
>> // will try to read one more roud when read error
>>
>> I suggest rewording this to:
>>
>> // will try to read one more time if there is a read error
>>
>> Also, it is too late to push this for JDK 8 as it is not critical, so
>> you will have to wait until the JDK 9 repo opens ...
>>
>> Thanks,
>> Sean
>>
>> On 12/04/2013 07:45 PM, zaiyao liu wrote:
>>> Hi Xuelei,
>>>
>>> Can you help to submit this change to repository? I don't have openjdk
>>> account.
>>>
>>> Thanks
>>>
>>> Kevin
>>> On 2013/12/4 15:41, Xuelei Fan wrote:
>>>> Looks fine to me.
>>>>
>>>> Xuelei
>>>>
>>>> On 12/4/2013 3:24 PM, zaiyao liu wrote:
>>>>> Hi Xuelei,
>>>>>
>>>>> I have updated, please
>>>>> review:http://cr.openjdk.java.net/~ewang/kevin/JDK-8028562/webrev.00/
>>>>> <http://cr.openjdk.java.net/%7Eewang/kevin/JDK-8028562/webrev.00/>
>>>>>
>>>>> Thanks
>>>>>
>>>>> Kevin
>>>>> On 2013/12/4 14:50, Xuelei Fan wrote:
>>>>>> On 12/4/2013 2:36 PM, zaiyao liu wrote:
>>>>>>> Hi Xuelei,
>>>>>>>
>>>>>>> Thanks for you suggestion. please review again:
>>>>>>> http://cr.openjdk.java.net/~ewang/kevin/JDK-8028562/webrev.00/
>>>>>>>
>>>>>> Need a white space:
>>>>>> - 224   //will try to read one more roud when read error
>>>>>> + 224   // will try to read one more roud when read error
>>>>>>
>>>>>> The message is not clear enough:
>>>>>> - 302  log("will read one more round");
>>>>>> + 302  log("Need to read more from client");
>>>>>>
>>>>>> Otherwise, looks fine to me.  Please go ahead.
>>>>>>
>>>>>> Thanks,
>>>>>> Xuelei
>>>>>>
>>>>>>> Kevin
>>>>>>> On 2013/12/4 12:06, Xuelei Fan wrote:
>>>>>>>> On 12/4/2013 11:33 AM, zaiyao liu wrote:
>>>>>>>>> Hi Xuelei,
>>>>>>>>>
>>>>>>>>> Can you help to review again.
>>>>>>>>> http://cr.openjdk.java.net/~ewang/kevin/JDK-8028562/webrev.00/
>>>>>>>>>
>>>>>>>> Thanks for the update.  Please pay attentions to the code
>>>>>>>> conversions.
>>>>>>>>
>>>>>>>>     300 if (serverIn.remaining() != clientMsg.length) {
>>>>>>>>     301     if(retry){
>>>>>>>>     302        log("will read one more round");
>>>>>>>>
>>>>>>>> It might be reasonable to retry when "serverIn.remaining()" less
>>>>>>>> than
>>>>>>>> clientMsg.length", what do you think?
>>>>>>>>
>>>>>>>>
>>>>>>>> Xuelei
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>> Kevin
>>>>>>>>> On 2013/12/3 19:50, Xuelei Fan wrote:
>>>>>>>>>> On 12/3/2013 6:59 PM, zaiyao liu wrote:
>>>>>>>>>>> Hi Xuelei,
>>>>>>>>>>>
>>>>>>>>>>> I can't reproduce this issue after run 900 times at windows and
>>>>>>>>>>> linux
>>>>>>>>>>> platform,
>>>>>>>>>> It should be pretty hard to reproduce the issue in normal TCP/IP
>>>>>>>>>> environment.
>>>>>>>>>>
>>>>>>>>>>> for this fix just run one more round after get exception.
>>>>>>>>>>>
>>>>>>>>>>> please review:
>>>>>>>>>>> http://sqeweb.us.oracle.com/net/sqenfs-1/export1/comp/jsn/users/kevin1/webrev/8028562/webrev/
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> I don't think it is the expected fix.  Looks like the underlying
>>>>>>>>>> issue
>>>>>>>>>> is that "serverOut.remaining() == 0" (line 282) does not always
>>>>>>>>>> mean
>>>>>>>>>> the
>>>>>>>>>> server has received all of the client message (line 298,
>>>>>>>>>> (serverIn.remaining() != clientMsg.length)).  I would suggest
>>>>>>>>>> run one
>>>>>>>>>> more round (at line 241) after server message delivered
>>>>>>>>>> ("serverOut.remaining() == 0" (line 282)).
>>>>>>>>>>
>>>>>>>>>> The logic looks like, in runTest(boolean):
>>>>>>>>>> loop (line 241):
>>>>>>>>>>        read client message
>>>>>>>>>>        send server message
>>>>>>>>>>        if server delivered all server message {
>>>>>>>>>>            if server received all client message {
>>>>>>>>>>                check the message
>>>>>>>>>>            } else {
>>>>>>>>>>                loop one more time, go to "loop" (only one time?).
>>>>>>>>>>            }
>>>>>>>>>>        }
>>>>>>>>>>
>>>>>>>>>> Hope it helps.
>>>>>>>>>>
>>>>>>>>>> Xuelei
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>>
>>>>>>>>>>> Kevin
>>>
>>
>