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

[Artem Smotrakov]

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

Here is an updated webrev:

http://cr.openjdk.java.net/~asmotrak/8162484/webrev.01/

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
>