Looks fine to me. Thanks, Xuelei
On 8/16/2016 1:31 AM, Artem Smotrakov wrote: > Hi Xuelei, > > Makes sense to me, I removed those 1 second delays > > http://cr.openjdk.java.net/~asmotrak/8162484/webrev.03/ > > Artem > > > On 08/12/2016 06:17 PM, Xuelei Fan wrote: >> 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 >
