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
