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
