Re: Code Review Request, JDK-8214339 : SSLSocketImpl erroneously wraps SocketException
Just the complete the thread. Says "fatal() throws exception" is fine.
If this all only trying to make the compiler happy, you could just have
one "return null" at the bottom of the method. It is not necessary to
put a return after each fatal(). I will admit it could be less readable.
Or fatal could return the exception, then the calling method would throw
it: throw shc.conContext.fatal();
But that's a larger change that I don't expected changed here.
Tony
On 12/14/18 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.
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
Code Review Request JDK-8215443: The use of TransportContext.fatal() leads to bad coding style
Hi, Could I have the update reviewed? http://cr.openjdk.java.net/~xuelei/8215443/webrev.00/ The TransportContext.fatal() methods always throw exception. While the compiler does not aware of it, and may not happy without following a return statement. Currently, a lot never executable return statements are inserted. As make the code hard to read (thanks for Jamil and Tony's points). For example: shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...); return null;// fatal() always throws, make the compiler happy. In this update, I changed the fatal() method with a return value: -void fatal(Alert alert, ... +SSLException fatal(Alert alert, ... Then we can change the use of method as: -shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...); -return null;// fatal() always throws, make the compiler happy. +throw shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...); The changeset is mostly about removing the never executed return statements and add the 'throw' keyword to lines that use the fatal() methods. Thanks, Xuelei
Re: 12 RFR(M) 8214583: AccessController.getContext may return wrong value after JDK-8212605
Hi Dean,
to avoid escape analysis-eliminated allocations in the past @DontInline
has been sufficient. This means a simpler patch (no changes to native
code needed - added assertions notwithstanding) and passes your tests
with C2 (it'd concern me if Graal's EA sees through this trick, as it
might break some existing places where DontInline is used to this
effect):
/**
* The value needs to be physically located in the frame, so that it
* can be found by a stack walk.
*/
@Hidden
@DontInline
private static void ensureMaterializedForStackWalk(Object o) {}
Thanks!
/Claes
On 2018-12-15 01:59, dean.l...@oracle.com wrote:
https://bugs.openjdk.java.net/browse/JDK-8214583
http://cr.openjdk.java.net/~dlong/8214583/webrev
This change includes two new regression test that demonstrate the
problem, and a fix that allows the tests
to pass.
The problem happens when the JIT compiler's escape analysis eliminates
the allocation of the AccessControlContext object passed to
doPrivileged. The compiler thinks this is safe because it does not see
that the object "escapes". However, getContext needs to be able to find
the object using a stack walk, so we need a way to tell the compiler
that it does indeed escape. To do this we pass the value to a native
method that does nothing.
Microbenchmark results:
jdk12-b18:
Benchmark Mode Cnt Score Error Units
DoPrivileged.test avgt 25 255.626 ± 6.446 ns/op
DoPrivileged.testInline avgt 25 250.968 ± 4.975 ns/op
jdk12-b19:
Benchmark Mode Cnt Score Error Units
DoPrivileged.test avgt 25 5.689 ± 0.001 ns/op
DoPrivileged.testInline avgt 25 2.765 ± 0.001 ns/op
this fix:
Benchmark Mode Cnt Score Error Units
DoPrivileged.test avgt 25 5.020 ± 0.001 ns/op
DoPrivileged.testInline avgt 25 2.774 ± 0.025 ns/op
dl
