Thank you Artem. The fix looks good. You just need a +1 from an official reviewer.
--Jamil -------- Original message --------From: Artem Smotrakov <artem.smotra...@oracle.com> Date: 8/12/16 1:07 PM (GMT-08:00) To: Jamil Nimeh <jamil.j.ni...@oracle.com>, Security Dev OpenJDK <security-dev@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@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 >>>>> >>>> >>> >> >
