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

[Jamil Nimeh]

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
>>>