[9] RFR: 8162484: javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails intermittently with "Address already in use" error

Xuelei Fan xuelei.fan at oracle.com
Mon Aug 15 23:29:13 UTC 2016 

Looks fine to me.

Thanks,
Xuelei

On 8/16/2016 1:31 AM, Artem Smotrakov wrote:
> Hi Xuelei,
> 
> Makes sense to me, I removed those 1 second delays
> 
> http://cr.openjdk.java.net/~asmotrak/8162484/webrev.03/
> 
> Artem
> 
> 
> On 08/12/2016 06:17 PM, Xuelei Fan wrote:
>> It's a nice find of the port reuse issues.
>>
>> This update will turn into expected connection failure into
>> reading/writing interruption as the server simulate the failure by
>> closing the incoming connections.  It's fine for this test,  I think.
>>
>> For lines like:
>>   288         intOcsp.rejectConnections();
>>   289         rootOcsp.rejectConnections();
>>   290         Thread.sleep(1000);
>>
>> I was wondering as the server does not need to bootup again, is the
>> delay still needed?
>>
>> Otherwise, looks fine to me.
>>
>> Thanks,
>> Xuelei
>>
>> On 8/13/2016 6:25 AM, Artem Smotrakov wrote:
>>> Thank you for review Jamil.
>>>
>>> Xuelei,
>>>
>>> Could you please take a look?
>>>
>>> Artem
>>>
>>>
>>> On 08/12/2016 02:38 PM, Jamil Nimeh wrote:
>>>> Thank you Artem.  The fix looks good.  You just need a +1 from an
>>>> official reviewer.
>>>>
>>>>
>>>>
>>>> --Jamil
>>>>
>>>> -------- Original message --------
>>>> From: Artem Smotrakov <artem.smotrakov at oracle.com>
>>>> Date: 8/12/16 1:07 PM (GMT-08:00)
>>>> To: Jamil Nimeh <jamil.j.nimeh at oracle.com>, Security Dev OpenJDK
>>>> <security-dev at openjdk.java.net>
>>>> Subject: Re: [9] RFR: 8162484:
>>>> javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails
>>>> intermittently with "Address already in use" error
>>>>
>>>> No problem.
>>>>
>>>> http://cr.openjdk.java.net/~asmotrak/8162484/webrev.02/
>>>>
>>>> Artem
>>>>
>>>>
>>>> On 08/12/2016 12:02 PM, Jamil Nimeh wrote:
>>>>> For the tests as we use them today we don't intend the server to
>>>>> restart.  The intent of SimpleOCSPServer was to be of use for a
>>>>> variety of testing purposes.  I don't know that we can say for all
>>>>> intended uses that we'll *never* need to restart it. That's why I'd
>>>>> like to keep the unbound socket/set sockopt/bind/listen behavior.  I
>>>>> don't think ServerSocket(0) achieves that.
>>>>>
>>>>> --Jamil
>>>>>
>>>>> On 8/12/2016 11:30 AM, Artem Smotrakov wrote:
>>>>>> Hi Jamil,
>>>>>>
>>>>>> There was no any specific reason to remove ServerSocket.bind() call.
>>>>>> ServerSocket(0) constructor creates a server socket, automatically
>>>>>> bound to a random free port. If I am not missing something, it
>>>>>> doesn't look necessary to set the SO_REUSEADDR socket options if the
>>>>>> server is not going to restart. The code is just shorter if we use
>>>>>> ServerSocket(0) constructor to open a server socket, but I can revert
>>>>>> it to use bind() with 0 port number if you think it's better.
>>>>>>
>>>>>> Artem
>>>>>>
>>>>>>
>>>>>> On 08/12/2016 09:13 AM, Jamil Nimeh wrote:
>>>>>>> Hi Artem, more comments in-line
>>>>>>>
>>>>>>>
>>>>>>> On 8/11/2016 11:46 AM, Artem Smotrakov wrote:
>>>>>>>> Hi Jamil,
>>>>>>>>
>>>>>>>> Thank you for review. Please see inline.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 08/10/2016 04:16 PM, Jamil Nimeh wrote:
>>>>>>>>> Hi Artem,
>>>>>>>>>
>>>>>>>>> I'm not an official reviewer but the solution for making the
>>>>>>>>> servers reject connections rather than stop and start looks pretty
>>>>>>>>> fair to me and seems like a nice way to simulate a downed OCSP
>>>>>>>>> responder instead of having to bounce it.  A couple
>>>>>>>>> comments/questions:
>>>>>>>>>
>>>>>>>>> I'm a bit surprised that you get the "Address already in use"
>>>>>>>>> error though.
>>>>>>>> Well, to be honest, I was not able to reproduce this failure
>>>>>>>> locally. I was running the test in a loop for a couple of days, and
>>>>>>>> it didn't fail. But the test has been observed to fail in other
>>>>>>>> test runs (jprt, CI, etc).
>>>>>>>>
>>>>>>>> I am not an expert in networking, and I would appreciate if someone
>>>>>>>> more knowledgeable gives an advise how these intermittent failures
>>>>>>>> can be avoided.
>>>>>>>>
>>>>>>>>> Isn't servSocket.setReuseAddress(true) on line 214 supposed to set
>>>>>>>>> the SO_REUSEADDR at the system call level and prevent EADDRINUSE
>>>>>>>>> when listening or binding?
>>>>>>>> If I am not missing something, the test has been observed to fails
>>>>>>>> while re-binding. I am wondering if it's possible that the port
>>>>>>>> becomes busy after the server socket was closed, but before bind()
>>>>>>>> is called again. The probability of this situation seems to be very
>>>>>>>> low which has been actually seen - the test fails very rare.
>>>>>>>>
>>>>>>>> If this is the case, it seems like servSocket.setReuseAddress(true)
>>>>>>>> doesn't help because the port is taken by another process (I am not
>>>>>>>> sure that SO_REUSEADDR prevents from this). Again, this is only my
>>>>>>>> guess, and I may be wrong.
>>>>>>> You know, I hadn't thought of that.  I've never been able to
>>>>>>> reproduce that problem either, but I'm running on a local virtual
>>>>>>> box VM on a laptop, and usually the tests are running sequentially.
>>>>>>> I could definitely see the case where other processes are soaking up
>>>>>>> the OCSP responder's port.  With those tests, I kind of need the
>>>>>>> port to remain the same since I'm putting that server and port in
>>>>>>> the AIA extensions of the certs for which it answers.  Given this
>>>>>>> particular case, it seems like your solution of keeping the server
>>>>>>> bound but just chopping connections off is the best way to go.
>>>>>>>>> When you create the new ServerSocket on line 212, you're now
>>>>>>>>> binding it to the port now where originally it started as an
>>>>>>>>> unbound socket.  By doing so, the behavior of setReuseAddress() on
>>>>>>>>> line 214 is now undefined.
>>>>>>>> This setReuseAddress() call looks unnecessary now. I'll update the
>>>>>>>> test.
>>>>>>>>> While this test no longer stops/starts the server, other tests may
>>>>>>>>> wish to do so and their behavior may not be consistent (though
>>>>>>>>> apparently it wasn't consistent even in the old scheme where the
>>>>>>>>> socket was unbound, then setReuseAddress() was called...)
>>>>>>>> Correct. I checked other code which depend on SimpleOCSPServer
>>>>>>>>
>>>>>>>> javax/net/ssl/Stapling/HttpsUrlConnClient.java
>>>>>>>> javax/net/ssl/Stapling/SSLEngineWithStapling.java
>>>>>>>> javax/net/ssl/Stapling/SSLSocketWithStapling.java
>>>>>>>> javax/net/ssl/Stapling/StapleEnableProps.java
>>>>>>>>
>>>> sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java
>>>>
>>>>
>>>>>>>>
>>>> sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java
>>>>
>>>>
>>>>>>>> artem at artem-laptop:~/ws/jdk/jdk9_dev_stapling_test/jdk/test$ kate
>>>>>>>> javax/net/ssl/Stapling/HttpsUrlConnClient.java
>>>>>>>> javax/net/ssl/Stapling/SSLEngineWithStapling.java
>>>>>>>> javax/net/ssl/Stapling/SSLSocketWithStapling.java
>>>>>>>> javax/net/ssl/Stapling/StapleEnableProps.java
>>>>>>>>
>>>> sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java
>>>>
>>>>
>>>> sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java
>>>>
>>>>
>>>>>>>>
>>>>>>>> These tests call stop() only once when actual testcases are done.
>>>>>>>> Actually, some of them don't even call stop(), but it seems to work
>>>>>>>> fine. As an enhancement, I would add stop() calls to finally
>>>>>>>> blocks, but it seems to work fine without it anyway.
>>>>>>> I liked your solution with the stop() calls in finally blocks and I
>>>>>>> agree that they should have them.  I think we get away with it
>>>>>>> because in most if not all of those cases they are running as
>>>>>>> othervm tests (because we have properties that we set specific to
>>>>>>> the tests).  So when the JVM exits resources like sockets are closed
>>>>>>> by the OS.  Still, it's better to have the try/finally guards and
>>>>>>> explicitly and gracefully shutdown the OCSP responders.
>>>>>>>> Here is an updated webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~asmotrak/8162484/webrev.01/
>>>>>>> I realize that in many of these test cases we're going to move away
>>>>>>> from a start/stop approach to your accept/reject one, but in general
>>>>>>> sockets designed to be listening should start unbound, set the
>>>>>>> SO_REUSEADDR sockopt, then bind and listen. Was there a specific
>>>>>>> reason to change that code, or was it just to streamline it?  Aside
>>>>>>> from fewer lines of code, I'm not sure what it buys us.
>>>>>>>> Artem
>>>>>>>>
>>>>>>>>
>>>>>>>>> --Jamil
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 08/10/2016 03:44 PM, Artem Smotrakov wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> Please review this update for OCSP stapling tests.
>>>>>>>>>>
>>>>>>>>>> The tests use
>>>>>>>>>> test/java/security/testlibrary/SimpleOCSPServer.java which try to
>>>>>>>>>> re-use a server port if the server restarted. Looks like
>>>>>>>>>> sometimes it may cause "Address already in use" error.
>>>>>>>>>>
>>>>>>>>>> The patch updates OCSP stapling tests with the following:
>>>>>>>>>> - updated SSLSocketWithStapling.java test not to restart OCSP
>>>>>>>>>> responders
>>>>>>>>>> - updated SimpleOCSPServer to be able to reject incoming
>>>> connections
>>>>>>>>>> - updated SimpleOCSPServer to be able to reproduce a delay
>>>>>>>>>> without restarting
>>>>>>>>>>
>>>>>>>>>> Jamil,
>>>>>>>>>>
>>>>>>>>>> Could you please take a look at this update, and confirm if this
>>>>>>>>>> update doesn't break the original test scenarios?
>>>>>>>>>>
>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8162484
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~asmotrak/8162484/webrev.00/
>>>>>>>>>>
>>>>>>>>>> Artem
>

More information about the security-dev mailing list