Merge lp:~rogpeppe/juju-core/213-rpc-test-timeouts into lp:~juju/juju-core/trunk

[Roger Peppe]

rpc: don't hang tests on failure

As suggested in previous CL.
Also delete Accept method, also suggested earlier
and not done.

https://codereview.appspot.com/7324046/

[Roger Peppe (rogpeppe)]

Reviewers: mp+148162_code.launchpad.net,

Message:
Please take a look.

Description:
rpc: don't hang tests on failure

https://code.launchpad.net/~rogpeppe/juju-core/213-rpc-test-timeouts/+merge/148162

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7324046/

Affected files:
   A [revision details]
   M rpc/rpc_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: rpc/rpc_test.go
=== modified file 'rpc/rpc_test.go'
--- rpc/rpc_test.go 2013-02-12 15:37:30 +0000
+++ rpc/rpc_test.go 2013-02-13 12:18:19 +0000
@@ -160,7 +160,7 @@
    }
   }
   client.Close()
- err := <-srvDone
+ err := chanReadError(c, srvDone, "server done")
   c.Assert(err, IsNil)
  }

@@ -211,29 +211,29 @@
   }

   client, srvDone := newRPCClientServer(c, root)
- call := func(id string, done chan<- bool) {
+ call := func(id string, done chan<- struct{}) {
    var r stringVal
    err := client.Call("DelayedMethods", id, "Delay", nil, &r)
    c.Check(err, IsNil)
    c.Check(r.Val, Equals, "return "+id)
- done <- true
+ done <- struct{}{}
   }
- done1 := make(chan bool)
- done2 := make(chan bool)
+ done1 := make(chan struct{})
+ done2 := make(chan struct{})
   go call("1", done1)
   go call("2", done2)

   // Check that both calls are running concurrently.
- <-ready1
- <-ready2
+ chanRead(c, ready1, "method 1 ready")
+ chanRead(c, ready2, "method 2 ready")

   // Let the requests complete.
   start1 <- "return 1"
   start2 <- "return 2"
- <-done1
- <-done2
+ chanRead(c, done1, "method 1 done")
+ chanRead(c, done2, "method 2 done")
   client.Close()
- err := <-srvDone
+ err := chanReadError(c, srvDone, "server done")
   c.Assert(err, IsNil)
  }

@@ -249,14 +249,14 @@
    },
   }
   client, srvDone := newRPCClientServer(c, root)
- done := make(chan bool)
+ done := make(chan struct{})
   go func() {
    var r stringVal
    err := client.Call("DelayedMethods", "1", "Delay", nil, &r)
    c.Check(err, FitsTypeOf, &net.OpError{})
- done <- true
+ done <- struct{}{}
   }()
- <-ready
+ chanRead(c, ready, "DelayedMethods.Delay ready")
   client.Close()
   select {
   case err := <-srvDone:
@@ -265,9 +265,28 @@
   case <-time.After(25 * time.Millisecond):
   }
   start <- "xxx"
- err := <-srvDone
+ err := chanReadError(c, srvDone, "server done")
   c.Check(err, IsNil)
- <-done
+ chanRead(c, done, "DelayedMethods.Delay done")
+}
+
+func chanRead(c *C, ch <-chan struct{}, what string) {
+ select {
+ case <-ch:
+ return
+ case <-time.After(3 * time.Second):
+ c.Fatalf("timeout on channel read %s", what)
+ }
+}
+
+func chanReadError(c *C, ch <-chan error, what string) error {
+ select {
+ case e := <-ch:
+ return e
+ case <-time.After(3 * time.Second):
+ c.Fatalf("timeout on channel read %s", what)
+ }
+ panic("unreachable")
  }

  // newRPCClientServer starts an RPC server serving a connection from a

[Dimiter Naydenov (dimitern)]

LGTM

https://codereview.appspot.com/7324046/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-02-13 16:40, Dimiter Naydenov wrote:
> LGTM
>
> https://codereview.appspot.com/7324046/
>

LGTM

I can confirm that it makes the tests fail quickly rather than hang
until the 5min timeout.

Of course, better if they didn't fail, but this is still an
improvement. :)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlEbiyQACgkQJdeBCYSNAAPYNQCgu/8iZnXoGba5+RMHDdo3XQoE
PYEAnRco0W3eMPh1BunHCHQ/DjYulkvF
=oBYC
-----END PGP SIGNATURE-----

[William Reade (fwereade)]

LGTM, one suggestion

https://codereview.appspot.com/7324046/diff/2001/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/7324046/diff/2001/rpc/rpc_test.go#newcode277
rpc/rpc_test.go:277: case <-time.After(3 * time.Second):
Can we reasonably drop this to, say, half a second?

https://codereview.appspot.com/7324046/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/7324046/

[Roger Peppe (rogpeppe)]

*** Submitted:

rpc: don't hang tests on failure

As suggested in previous CL.
Also delete Accept method, also suggested earlier
and not done.

R=dimitern, fwereade
CC=
https://codereview.appspot.com/7324046

https://codereview.appspot.com/7324046/diff/2001/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/7324046/diff/2001/rpc/rpc_test.go#newcode277
rpc/rpc_test.go:277: case <-time.After(3 * time.Second):
On 2013/02/13 12:56:35, fwereade wrote:
> Can we reasonably drop this to, say, half a second?

why bother? the timeout only happens on test failure, and in that case 3
seconds isn't an issue.

https://codereview.appspot.com/7324046/

=== modified file 'rpc/rpc_test.go'
--- rpc/rpc_test.go	2013-02-12 15:37:30 +0000
+++ rpc/rpc_test.go	2013-02-13 13:05:28 +0000
@@ -160,7 +160,7 @@
 		}
 	}
 	client.Close()
-	err := <-srvDone
+	err := chanReadError(c, srvDone, "server done")
 	c.Assert(err, IsNil)
 }
 
@@ -211,29 +211,29 @@
 	}
 
 	client, srvDone := newRPCClientServer(c, root)
-	call := func(id string, done chan<- bool) {
+	call := func(id string, done chan<- struct{}) {
 		var r stringVal
 		err := client.Call("DelayedMethods", id, "Delay", nil, &r)
 		c.Check(err, IsNil)
 		c.Check(r.Val, Equals, "return "+id)
-		done <- true
+		done <- struct{}{}
 	}
-	done1 := make(chan bool)
-	done2 := make(chan bool)
+	done1 := make(chan struct{})
+	done2 := make(chan struct{})
 	go call("1", done1)
 	go call("2", done2)
 
 	// Check that both calls are running concurrently.
-	<-ready1
-	<-ready2
+	chanRead(c, ready1, "method 1 ready")
+	chanRead(c, ready2, "method 2 ready")
 
 	// Let the requests complete.
 	start1 <- "return 1"
 	start2 <- "return 2"
-	<-done1
-	<-done2
+	chanRead(c, done1, "method 1 done")
+	chanRead(c, done2, "method 2 done")
 	client.Close()
-	err := <-srvDone
+	err := chanReadError(c, srvDone, "server done")
 	c.Assert(err, IsNil)
 }
 
@@ -249,14 +249,14 @@
 		},
 	}
 	client, srvDone := newRPCClientServer(c, root)
-	done := make(chan bool)
+	done := make(chan struct{})
 	go func() {
 		var r stringVal
 		err := client.Call("DelayedMethods", "1", "Delay", nil, &r)
 		c.Check(err, FitsTypeOf, &net.OpError{})
-		done <- true
+		done <- struct{}{}
 	}()
-	<-ready
+	chanRead(c, ready, "DelayedMethods.Delay ready")
 	client.Close()
 	select {
 	case err := <-srvDone:
@@ -265,9 +265,28 @@
 	case <-time.After(25 * time.Millisecond):
 	}
 	start <- "xxx"
-	err := <-srvDone
+	err := chanReadError(c, srvDone, "server done")
 	c.Check(err, IsNil)
-	<-done
+	chanRead(c, done, "DelayedMethods.Delay done")
+}
+
+func chanRead(c *C, ch <-chan struct{}, what string) {
+	select {
+	case <-ch:
+		return
+	case <-time.After(3 * time.Second):
+		c.Fatalf("timeout on channel read %s", what)
+	}
+}
+
+func chanReadError(c *C, ch <-chan error, what string) error {
+	select {
+	case e := <-ch:
+		return e
+	case <-time.After(3 * time.Second):
+		c.Fatalf("timeout on channel read %s", what)
+	}
+	panic("unreachable")
 }
 
 // newRPCClientServer starts an RPC server serving a connection from a

=== modified file 'rpc/server.go'
--- rpc/server.go	2013-02-11 12:30:10 +0000
+++ rpc/server.go	2013-02-13 13:05:28 +0000
@@ -4,7 +4,6 @@
 	"fmt"
 	"io"
 	"launchpad.net/juju-core/log"
-	"net"
 	"reflect"
 	"sync"
 )
@@ -61,44 +60,6 @@
 	sending sync.Mutex
 }
 
-// Accept accepts connections on the listener and serves requests for
-// each incoming connection.  The newRoot function is called
-// to create the root value for the connection before spawning
-// the goroutine to service the RPC requests; it may be nil,
-// in which case the original root value passed to NewServer
-// will be used. A codec is chosen for the connection by
-// calling newCodec.
-//
-// Accept blocks; the caller typically invokes it in
-// a go statement.
-func (srv *Server) Accept(l net.Listener,
-	newRoot func(net.Conn) (interface{}, error),
-	newCodec func(io.ReadWriteCloser) ServerCodec) error {
-	for {
-		c, err := l.Accept()
-		if err != nil {
-			return err
-		}
-		rootv := srv.root
-		if newRoot != nil {
-			root, err := newRoot(c)
-			if err != nil {
-				log.Printf("rpc: connection refused: %v", err)
-				c.Close()
-				continue
-			}
-			rootv = reflect.ValueOf(root)
-		}
-		go func() {
-			defer c.Close()
-			if err := srv.serve(rootv, newCodec(c)); err != nil {
-				log.Printf("rpc: ServeCodec error: %v", err)
-			}
-		}()
-	}
-	panic("unreachable")
-}
-
 // ServeCodec runs the server on a single connection.  ServeCodec
 // blocks, serving the connection until the client hangs up.  The caller
 // typically invokes ServeCodec in a go statement.  The given