On 1/11/2016 10:50 AM, joe darcy wrote: > Hi Xuelei, > > I'm not a concurrency expert, but I don't think it is proper to only > synchronize on the writing of a data structure -- the reading should be > synchronized too. > Yes. The fix considered the reading side too. The reading of the serverPorts will not happen unless the write side finished. The behavior is controlled by the "serverReady" variable. Therefore, it is OK to synchronize the write side of serverPorts in the test case.
The problem is mainly about the updated of the counter variable createdPorts. Both the reading and writing sides are synchronized in the same block. > Have you reproduced the failure with the code and seen the failure go > away with the new code? Is there a hypothesis on why the failure started > to happen recently when the test doesn't seem to have been modified? > It used to failure intermittent. But I cannot find the root cause in the past. So a socket accept timeout was added in order to find the underlying issues of the intermittent failure. The socket accept timeout would increase the frequency of the failure. And it did help to expose the root cause of the failure. > As a code review comment, I recommend in the doServerSide method using a > try-with-resources statement to manage the sslServerSocket variable. > It's a better style. I will make the update. > For a larger refactoring, is an array with a separate createdPorts count > the best data structure to use here? > It could be more clear, I think. But need too much refactoring, and may introduce new synchronization requirement. I will like to try this simple update at first, and see what happens. Thanks, Xuelei > Thanks, > > -Joe > > On 1/10/2016 4:11 PM, Xuelei Fan wrote: >> Looks fine to me. >> >> BTW, the fix for JDK-8146387 is asking for code review. >> >> http://mail.openjdk.java.net/pipermail/security-dev/2016-January/013301.html >> >> >> Thanks, >> Xuelei >> >> On 1/11/2016 4:54 AM, joe darcy wrote: >>> Hello, >>> >>> The test >>> >>> javax/net/ssl/SSLSession/SessionCacheSizeTests.java >>> >>> has been failing intermittently with high-frequency on Solaris and >>> Windows of late. The test should be problem listed until the underlying >>> problem is fixed (JDK-8146387). >>> >>> Patch to do this below. >>> >>> Thanks, >>> >>> -Joe >>> >>> diff -r aa9fd2797b82 test/ProblemList.txt >>> --- a/test/ProblemList.txt Sun Jan 10 11:09:31 2016 -0800 >>> +++ b/test/ProblemList.txt Sun Jan 10 12:53:16 2016 -0800 >>> @@ -1,6 +1,6 @@ >>> >>> ########################################################################### >>> >>> >>> # >>> -# Copyright (c) 2009, 2015, Oracle and/or its affiliates. All rights >>> reserved. >>> +# Copyright (c) 2009, 2016, Oracle and/or its affiliates. All rights >>> reserved. >>> # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. >>> # >>> # This code is free software; you can redistribute it and/or modify it >>> @@ -299,6 +299,9 @@ >>> # 8074580 >>> sun/security/pkcs11/rsa/TestKeyPairGenerator.java generic-all >>> >>> +# 8146387 >>> +javax/net/ssl/SSLSession/SessionCacheSizeTests.java >>> windows-all,solaris-all >>> + >>> >>> ############################################################################ >>> >>> >>> >>> # jdk_sound >>> diff -r aa9fd2797b82 >>> test/javax/net/ssl/SSLSession/SessionCacheSizeTests.java >>> --- a/test/javax/net/ssl/SSLSession/SessionCacheSizeTests.java Sun Jan >>> 10 11:09:31 2016 -0800 >>> +++ b/test/javax/net/ssl/SSLSession/SessionCacheSizeTests.java Sun Jan >>> 10 12:53:16 2016 -0800 >>> @@ -31,6 +31,7 @@ >>> * @bug 4366807 >>> * @summary Need new APIs to get/set session timeout and session cache >>> size. >>> * @run main/othervm SessionCacheSizeTests >>> + * @key intermittent >>> */ >>> >>> import java.io.*; >>> >>> >
