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