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