Code Review Request, JDK-8214339 : SSLSocketImpl erroneously wraps SocketException

[Xue-Lei Fan]

Hi Jamil,

Thanks for the code review.

Okay, I will go ahead with the comment "fatal() always throws, make the
compiler happy" for this update, and file a new bug and see if we could 
make an improvement in the future.

Thanks,
Xuelei

On 12/14/2018 4:16 PM, Jamil Nimeh wrote:
> Hi Xuelei, thanks for the clarification.  If you want to keep the 
> structure as-is, then the comment "fatal() always throws, make the 
> compiler happy" is fine.  I think that's a little more helpful to a 
> future maintainer who might be new to the code.
> 
> Thanks,
> --Jamil
> 
> On 12/14/2018 1:56 PM, Xue-Lei Fan wrote:
>> On 12/14/2018 1:46 PM, Xue-Lei Fan wrote:
>>> Hi,
>>>
>>> The purpose of combination of the two lines together:
>>>
>>>      shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, "...");
>>>      return null;    // make the complier happy
>>>
>>> is actually for the reading of the code.  If one don't know the 
>>> fatal() always throw an exception (compiler is one of them), he may 
>>> continue reading the following code, and may be confusing about the 
>>> code logic.
>>>
>>> The "return" statement could never be executed actually.  It could be 
>>> easier to catch the idea if using a format like:
>>>
>>>     throw new SSLException("there is an alert")
>>> -  return null;
>>>
>>> But we need the fatal() method to wrap something more.
>>>
>>> We used a lot comments similar to:
>>>
>>>     return;     // fatal() always throws, make the compiler happy.
>>>
>>> and the abbreviate comment:
>>>     return;    // make the compiler happy
>>>
>>> misses the part "fatal() always throws", and then it may look weird.
>>>
>>> I'm fine to remove the comment, or use the full comment instead 
>>> "fatal() always throws, make the compiler happy".  Which one is your 
>>> preference?
>>>
>>>
>>> On 12/14/2018 11:49 AM, Jamil Nimeh wrote:
>>>> Looks pretty good.  I did have one question about a few of the 
>>>> methods in KeyShareExtension and PreSharedKeyExtension, specifically 
>>>> where you return nulls on error branches with the 
>>>> make-compiler-happy comments. In those cases would it be a bit 
>>>> cleaner to set a byte[] variable to null at the beginning of the 
>>>> method and then in the successful code path set the value to 
>>>> whatever byte array comes back from hmacs or hkdf operations?  At 
>>>> the end of the method you only need one return statement, rather 
>>>> than a return null on every error branch, many of which won't be 
>>>> executed due to method calls which always throw exceptions.
>>> Hm, I see your points.  As the return statement could never be 
>>> executed, it may not worthy to declare the byte[] variable earlier.
>>>
>> I mean that we can remove the "return" statement as it is never 
>> executed, if not for code reading or compiler.
>>
>> In some other places, the compiler may not happy because it cannot 
>> tell that the fatal() throws exception.  So we may still need the 
>> "return" statement a little bit.  But I think most of them could be 
>> removed if we don't like it.
>>
>> Xuelei
>>
>>> See above. Is it a reasonable coding style to you as well?
>>>
>>> Thanks,
>>> Xuelei
>>>
>>>> Not a big deal if you wish to leave things as they are since I know 
>>>> the current approach is used in many places in the code that this 
>>>> review doesn't touch.
>>>>
>>>> --Jamil
>>>>
>>>> On 12/14/2018 8:14 AM, Xue-Lei Fan wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the update:
>>>>>    http://cr.openjdk.java.net/~xuelei/8214339/webrev.00/
>>>>>
>>>>> In some cases, the SSLProtocolException or SSLHandshakeException 
>>>>> may be thrown if the underlying socket run into problems.  An 
>>>>> application may depends on the exception class for further action, 
>>>>> for example retry the connection with different parameters.
>>>>>
>>>>> This update is trying to separate the socket problem from the TLS 
>>>>> protocol or handshake problem, by using different exception classes.
>>>>>
>>>>> Thanks,
>>>>> Xuelei
>>>>
>