<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Thank you for review Jamil.</p>
<p>Xuelei,</p>
<p>Could you please take a look?</p>
<p>Artem<br>
</p>
<br>
<div class="moz-cite-prefix">On 08/12/2016 02:38 PM, Jamil Nimeh
wrote:<br>
</div>
<blockquote
cite="mid:72walb7qsg2a2j6l62xtybpd.1471037888111@email.android.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div>Thank you Artem. The fix looks good. You just need a +1
from an official reviewer.</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div id="composer_signature">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
--Jamil</div>
<div><br>
</div>
<div style="font-size:100%;color:#000000"><!-- originalMessage -->
<div>-------- Original message --------</div>
<div>From: Artem Smotrakov <a class="moz-txt-link-rfc2396E" href="mailto:artem.smotrakov@oracle.com"><artem.smotrakov@oracle.com></a> </div>
<div>Date: 8/12/16 1:07 PM (GMT-08:00) </div>
<div>To: Jamil Nimeh <a class="moz-txt-link-rfc2396E" href="mailto:jamil.j.nimeh@oracle.com"><jamil.j.nimeh@oracle.com></a>, Security
Dev OpenJDK <a class="moz-txt-link-rfc2396E" href="mailto:security-dev@openjdk.java.net"><security-dev@openjdk.java.net></a> </div>
<div>Subject: Re: [9] RFR: 8162484:
javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails
intermittently with "Address already in use" error </div>
<div><br>
</div>
</div>
No problem.<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~asmotrak/8162484/webrev.02/">http://cr.openjdk.java.net/~asmotrak/8162484/webrev.02/</a><br>
<br>
Artem<br>
<br>
<br>
On 08/12/2016 12:02 PM, Jamil Nimeh wrote:<br>
> For the tests as we use them today we don't intend the server
to <br>
> restart. The intent of SimpleOCSPServer was to be of use for
a <br>
> variety of testing purposes. I don't know that we can say
for all <br>
> intended uses that we'll *never* need to restart it. That's
why I'd <br>
> like to keep the unbound socket/set sockopt/bind/listen
behavior. I <br>
> don't think ServerSocket(0) achieves that.<br>
><br>
> --Jamil<br>
><br>
> On 8/12/2016 11:30 AM, Artem Smotrakov wrote:<br>
>> Hi Jamil,<br>
>><br>
>> There was no any specific reason to remove
ServerSocket.bind() call. <br>
>> ServerSocket(0) constructor creates a server socket,
automatically <br>
>> bound to a random free port. If I am not missing
something, it <br>
>> doesn't look necessary to set the SO_REUSEADDR socket
options if the <br>
>> server is not going to restart. The code is just shorter
if we use <br>
>> ServerSocket(0) constructor to open a server socket, but
I can revert <br>
>> it to use bind() with 0 port number if you think it's
better.<br>
>><br>
>> Artem<br>
>><br>
>><br>
>> On 08/12/2016 09:13 AM, Jamil Nimeh wrote:<br>
>>> Hi Artem, more comments in-line<br>
>>><br>
>>><br>
>>> On 8/11/2016 11:46 AM, Artem Smotrakov wrote:<br>
>>>> Hi Jamil,<br>
>>>><br>
>>>> Thank you for review. Please see inline.<br>
>>>><br>
>>>><br>
>>>> On 08/10/2016 04:16 PM, Jamil Nimeh wrote:<br>
>>>>> Hi Artem,<br>
>>>>><br>
>>>>> I'm not an official reviewer but the solution
for making the <br>
>>>>> servers reject connections rather than stop
and start looks pretty <br>
>>>>> fair to me and seems like a nice way to
simulate a downed OCSP <br>
>>>>> responder instead of having to bounce it. A
couple <br>
>>>>> comments/questions:<br>
>>>>><br>
>>>>> I'm a bit surprised that you get the "Address
already in use" <br>
>>>>> error though. <br>
>>>> Well, to be honest, I was not able to reproduce
this failure <br>
>>>> locally. I was running the test in a loop for a
couple of days, and <br>
>>>> it didn't fail. But the test has been observed to
fail in other <br>
>>>> test runs (jprt, CI, etc).<br>
>>>><br>
>>>> I am not an expert in networking, and I would
appreciate if someone <br>
>>>> more knowledgeable gives an advise how these
intermittent failures <br>
>>>> can be avoided.<br>
>>>><br>
>>>>> Isn't servSocket.setReuseAddress(true) on
line 214 supposed to set <br>
>>>>> the SO_REUSEADDR at the system call level and
prevent EADDRINUSE <br>
>>>>> when listening or binding?<br>
>>>> If I am not missing something, the test has been
observed to fails <br>
>>>> while re-binding. I am wondering if it's possible
that the port <br>
>>>> becomes busy after the server socket was closed,
but before bind() <br>
>>>> is called again. The probability of this
situation seems to be very <br>
>>>> low which has been actually seen - the test fails
very rare.<br>
>>>><br>
>>>> If this is the case, it seems like
servSocket.setReuseAddress(true) <br>
>>>> doesn't help because the port is taken by another
process (I am not <br>
>>>> sure that SO_REUSEADDR prevents from this).
Again, this is only my <br>
>>>> guess, and I may be wrong.<br>
>>> You know, I hadn't thought of that. I've never been
able to <br>
>>> reproduce that problem either, but I'm running on a
local virtual <br>
>>> box VM on a laptop, and usually the tests are running
sequentially. <br>
>>> I could definitely see the case where other processes
are soaking up <br>
>>> the OCSP responder's port. With those tests, I kind
of need the <br>
>>> port to remain the same since I'm putting that server
and port in <br>
>>> the AIA extensions of the certs for which it
answers. Given this <br>
>>> particular case, it seems like your solution of
keeping the server <br>
>>> bound but just chopping connections off is the best
way to go.<br>
>>>>><br>
>>>>> When you create the new ServerSocket on line
212, you're now <br>
>>>>> binding it to the port now where originally
it started as an <br>
>>>>> unbound socket. By doing so, the behavior of
setReuseAddress() on <br>
>>>>> line 214 is now undefined.<br>
>>>> This setReuseAddress() call looks unnecessary
now. I'll update the <br>
>>>> test.<br>
>>>>> While this test no longer stops/starts the
server, other tests may <br>
>>>>> wish to do so and their behavior may not be
consistent (though <br>
>>>>> apparently it wasn't consistent even in the
old scheme where the <br>
>>>>> socket was unbound, then setReuseAddress()
was called...)<br>
>>>> Correct. I checked other code which depend on
SimpleOCSPServer<br>
>>>><br>
>>>> javax/net/ssl/Stapling/HttpsUrlConnClient.java<br>
>>>> javax/net/ssl/Stapling/SSLEngineWithStapling.java<br>
>>>> javax/net/ssl/Stapling/SSLSocketWithStapling.java<br>
>>>> javax/net/ssl/Stapling/StapleEnableProps.java<br>
>>>>
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java
<br>
>>>><br>
>>>>
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java
<br>
>>>><br>
>>>>
artem@artem-laptop:~/ws/jdk/jdk9_dev_stapling_test/jdk/test$ kate
<br>
>>>> javax/net/ssl/Stapling/HttpsUrlConnClient.java <br>
>>>> javax/net/ssl/Stapling/SSLEngineWithStapling.java
<br>
>>>> javax/net/ssl/Stapling/SSLSocketWithStapling.java
<br>
>>>> javax/net/ssl/Stapling/StapleEnableProps.java <br>
>>>>
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java
<br>
>>>>
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java
<br>
>>>><br>
>>>><br>
>>>> These tests call stop() only once when actual
testcases are done. <br>
>>>> Actually, some of them don't even call stop(),
but it seems to work <br>
>>>> fine. As an enhancement, I would add stop() calls
to finally <br>
>>>> blocks, but it seems to work fine without it
anyway.<br>
>>> I liked your solution with the stop() calls in
finally blocks and I <br>
>>> agree that they should have them. I think we get
away with it <br>
>>> because in most if not all of those cases they are
running as <br>
>>> othervm tests (because we have properties that we set
specific to <br>
>>> the tests). So when the JVM exits resources like
sockets are closed <br>
>>> by the OS. Still, it's better to have the
try/finally guards and <br>
>>> explicitly and gracefully shutdown the OCSP
responders.<br>
>>>><br>
>>>> Here is an updated webrev:<br>
>>>><br>
>>>>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~asmotrak/8162484/webrev.01/">http://cr.openjdk.java.net/~asmotrak/8162484/webrev.01/</a><br>
>>> I realize that in many of these test cases we're
going to move away <br>
>>> from a start/stop approach to your accept/reject one,
but in general <br>
>>> sockets designed to be listening should start
unbound, set the <br>
>>> SO_REUSEADDR sockopt, then bind and listen. Was there
a specific <br>
>>> reason to change that code, or was it just to
streamline it? Aside <br>
>>> from fewer lines of code, I'm not sure what it buys
us.<br>
>>>><br>
>>>> Artem<br>
>>>><br>
>>>><br>
>>>>><br>
>>>>> --Jamil<br>
>>>>><br>
>>>>><br>
>>>>> On 08/10/2016 03:44 PM, Artem Smotrakov
wrote:<br>
>>>>>> Hello,<br>
>>>>>><br>
>>>>>> Please review this update for OCSP
stapling tests.<br>
>>>>>><br>
>>>>>> The tests use <br>
>>>>>>
test/java/security/testlibrary/SimpleOCSPServer.java which try to
<br>
>>>>>> re-use a server port if the server
restarted. Looks like <br>
>>>>>> sometimes it may cause "Address already
in use" error.<br>
>>>>>><br>
>>>>>> The patch updates OCSP stapling tests
with the following:<br>
>>>>>> - updated SSLSocketWithStapling.java test
not to restart OCSP <br>
>>>>>> responders<br>
>>>>>> - updated SimpleOCSPServer to be able to
reject incoming connections<br>
>>>>>> - updated SimpleOCSPServer to be able to
reproduce a delay <br>
>>>>>> without restarting<br>
>>>>>><br>
>>>>>> Jamil,<br>
>>>>>><br>
>>>>>> Could you please take a look at this
update, and confirm if this <br>
>>>>>> update doesn't break the original test
scenarios?<br>
>>>>>><br>
>>>>>> Bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8162484">https://bugs.openjdk.java.net/browse/JDK-8162484</a><br>
>>>>>> Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~asmotrak/8162484/webrev.00/">http://cr.openjdk.java.net/~asmotrak/8162484/webrev.00/</a><br>
>>>>>><br>
>>>>>> Artem<br>
>>>>><br>
>>>><br>
>>><br>
>><br>
><br>
<br>
</blockquote>
<br>
</body>
</html>