Code Review Request: 8017779: java/net/Authenticator/B4769350.java fails
Andreas Rieber
rieberandreas at gmail.com
Fri Jul 26 09:50:33 PDT 2013
Hi Kurchi,
i looked into the change. Today i did run it a few time and it allays
worked. The logic looks straight forward to me.
I would only improve indenting and the try - autoclose for the Server.
The startServer() can be implemented in the constructor. The
server.close() in the exept() method is not required due to the
autoclose and this saves a few parameters here and there.
cheers
Andreas
Here my diff on your patch change:
diff --git a/test/java/net/Authenticator/B4769350.java
b/test/java/net/Authenticator/B4769350.java
--- a/test/java/net/Authenticator/B4769350.java
+++ b/test/java/net/Authenticator/B4769350.java
@@ -55,7 +55,7 @@
}
}
- static class Client extends Thread {
+ static class Client extends Thread {
String authority, path;
boolean allowerror;
@@ -74,7 +74,7 @@
InputStream is = urlc.getInputStream();
read (is);
is.close();
- } catch (URISyntaxException e) {
+ } catch (URISyntaxException e) {
System.out.println (e);
error = true;
} catch (IOException e) {
@@ -94,11 +94,7 @@
CyclicBarrier t1Cond1;
CyclicBarrier t1Cond2;
- public String getAddress() {
- return server.getAddress().getHostName();
- }
-
- public void startServer() {
+ public Server() {
InetSocketAddress addr = new InetSocketAddress(0);
try {
@@ -131,6 +127,10 @@
server.start();
}
+ public String getAddress() {
+ return server.getAddress().getHostName();
+ }
+
public int getPort() {
return server.getAddress().getPort();
}
@@ -143,9 +143,10 @@
}
/* T1 tests the client by sending 4 requests to 2 different realms
- * in parallel. The client should recognise two pairs of
dependent requests
- * and execute the first of each pair in parallel. When they
both succeed
- * the second requests should be executed without calling the
authenticator.
+ * in parallel. The client should recognise two pairs of dependent
+ * requests and execute the first of each pair in parallel.
When they
+ * both succeed the second requests should be executed without
calling
+ * the authenticator.
* The test succeeds if the authenticator was only called twice.
*/
class AuthenticationHandlerT1a implements HttpHandler
@@ -171,11 +172,9 @@
default:
System.out.println ("Unexpected request");
}
- } catch (InterruptedException |
- BrokenBarrierException e)
- {
- throw new RuntimeException(e);
- }
+ } catch (InterruptedException | BrokenBarrierException e) {
+ throw new RuntimeException(e);
+ }
}
}
@@ -270,10 +269,9 @@
}
}
- /* T2 tests to check that if initial authentication fails, the
second will
- * succeed, and the authenticator is called twice
+ /* T2 tests to check that if initial authentication fails, the
second
+ * will succeed, and the authenticator is called twice
*/
-
class AuthenticationHandlerT2a implements HttpHandler
{
volatile int count = -1;
@@ -287,11 +285,10 @@
}
AuthenticationHandler.errorReply(exchange,
"Basic realm=\"realm3\"");
-
}
}
- class AuthenticationHandlerT2b implements HttpHandler
+ class AuthenticationHandlerT2b implements HttpHandler
{
volatile int count = -1;
@@ -316,7 +313,6 @@
* resource at same time. Authenticator should be called once
for server
* and once for proxy
*/
-
class AuthenticationHandlerT3a implements HttpHandler
{
volatile int count = -1;
@@ -423,11 +419,10 @@
int f = auth.getCount();
if (f != 2) {
- except ("Authenticator was called "+f+" times. Should be 2",
- server);
+ except ("Authenticator was called "+f+" times. Should be 2");
}
if (error) {
- except ("error occurred", server);
+ except ("error occurred");
}
auth.resetCount();
@@ -444,10 +439,10 @@
f = auth.getCount();
if (f != redirects+1) {
except ("Authenticator was called "+f+" times. Should be: "
- + redirects+1, server);
+ + redirects+1);
}
if (error) {
- except ("error occurred", server);
+ except ("error occurred");
}
}
@@ -466,11 +461,10 @@
int f = auth.getCount();
if (f != 2) {
- except ("Authenticator was called "+f+" times. Should be: "
+ 2,
- server);
+ except ("Authenticator was called "+f+" times. Should be: "
+ 2);
}
if (error) {
- except ("error occurred", server);
+ except ("error occurred");
}
}
@@ -482,10 +476,11 @@
System.setProperty ("http.maxRedirects", Integer.toString
(redirects));
System.setProperty ("http.auth.serializeRequests", "true");
Authenticator.setDefault (auth);
+
try (Server server = new Server()) {
- server.startServer();
System.out.println ("Server: listening on port: "
+ server.getPort());
+
if (proxy) {
System.setProperty ("http.proxyHost", "localhost");
System.setProperty ("http.proxyPort",
@@ -495,11 +490,9 @@
doServerTests ("localhost:"+server.getPort(), server);
}
}
-
}
- public static void except (String s, Server server) {
- server.close();
+ public static void except (String s) {
throw new RuntimeException (s);
}
On 25.07.13 20:35, Kurchi Subhra Hazra wrote:
> Hi,
>
> Did anyone have a chance to look at this?
>
> Thanks,
> Kurchi
>
>
> On Thu, Jul 18, 2013 at 4:26 PM, Kurchi Hazra
> <kurchi.subhra.hazra at oracle.com <mailto:kurchi.subhra.hazra at oracle.com>>
> wrote:
>
> Hi Michael,
>
> I added some comments as to what is the purpose of the latches
> and barriers. Those comments alongwith the
> comments describing the purpose of the handlers should make the
> synchronization logic more clear. Let me know what
> you think: http://cr.openjdk.java.net/~__khazra/8017779/webrev.01/
> <http://cr.openjdk.java.net/~khazra/8017779/webrev.01/>
>
> Thanks,
> Kurchi
>
> On 7/17/2013 2:07 PM, Kurchi Hazra wrote:
>
>
> On 7/17/2013 12:27 AM, Michael McMahon wrote:
>
> On 16/07/13 20:11, Kurchi Hazra wrote:
>
> Hi,
> We have observed that
> test/java/net/Authenticator/__B4769350.java fails
> intermittently. Although not reproducible always,
> the bug could be in the test/sun/net/www/httptest
> library that this test uses. I have rewritten the test
> to use
> com.sun.net.httpserver instead since we anyway want to
> move away from using the httptest library.
>
> I have used CyclicBarriers to mimic
> TestHttpServer.rendezvous() and CountDownLatches to
> mimic TestHttpServer.__waitForCondition() and hopefully
> preserved the rest of the logic in the test.
> I have not seen the test failing after these changes.
>
> Bug: http://bugs.sun.com/view_bug.__do?bug_id=8017779
> <http://bugs.sun.com/view_bug.do?bug_id=8017779>
> Webrev:
> http://cr.openjdk.java.net/~__khazra/8017779/webrev.00/
> <http://cr.openjdk.java.net/~khazra/8017779/webrev.00/>
>
> Thanks,
> Kurchi
>
>
> Kurchi,
>
> Since this is a fairly complicated test, and it's great to
> see it being rewritten,
> is there any possibility of adding some commentary that
> explains the purpose
> of the synchronization code. For instance, I can't see the
> purpose of the call
> on line 163 as it just blocks a thread that has already
> completed its work
>
> Michael
>
>
> Hi Michael,
>
> I have just preserved whatever logic the test originally had.
> The specific instance you point out waits
> for the T1b() handle to be executed for case 0 before moving
> forward. But I guess it is hard to understand in a
> glance and I'll add some more comments and get back with a new
> webrev.
>
> Thanks,
> Kurchi
>
>
> --
> -Kurchi
>
>
More information about the net-dev
mailing list