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

Fri Aug 12 18:30:06 UTC 2016

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