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

Sat Aug 13 01:17:21 UTC 2016

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