Skip to content

Commit

Permalink
rpc: remove shutdown deadlock
Browse files Browse the repository at this point in the history
shutdown was originally written for Conn.Close and included a wait for
goroutines to report done, as is appropriate.  However, when shutdown
was called from one of the manager's goroutines (in case of an abort,
etc.), the goroutines would deadlock.  Now manager.wait is separated out,
and I made Conn.Wait use the manager.wait function.  This has the
side-effect of making Conn.Wait wait for the goroutines to finish, which
is more correct than the old behavior.

Fixes #45
  • Loading branch information
zombiezen committed Aug 15, 2016
1 parent 5aac86f commit f6d1181
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 7 deletions.
16 changes: 10 additions & 6 deletions rpc/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ func (m *manager) do(f func()) {
}
}

// shutdown closes the finish channel and sets the error.
// The first call to shutdown returns true; subsequent calls are no-ops
// and return false.
// shutdown closes the finish channel and sets the error. The first
// call to shutdown returns true; subsequent calls are no-ops and return
// false. This will not wait for the manager's goroutines to finish.
func (m *manager) shutdown(e error) bool {
m.mu.Lock()
ok := !m.done
Expand All @@ -64,12 +64,16 @@ func (m *manager) shutdown(e error) bool {
m.e = e
}
m.mu.Unlock()
if ok {
m.wg.Wait()
}
return ok
}

// wait blocks until the manager is shut down and all of its goroutines
// are finished.
func (m *manager) wait() {
<-m.finish
m.wg.Wait()
}

// err returns the error passed to shutdown.
func (m *manager) err() error {
m.mu.RLock()
Expand Down
3 changes: 2 additions & 1 deletion rpc/rpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func NewConn(t Transport, options ...ConnOption) *Conn {
// Wait waits until the connection is closed or aborted by the remote vat.
// Wait will always return an error, usually ErrConnClosed or of type Abort.
func (c *Conn) Wait() error {
<-c.manager.finish
c.manager.wait()
return c.manager.err()
}

Expand All @@ -139,6 +139,7 @@ func (c *Conn) Close() error {
if !c.manager.shutdown(ErrConnClosed) {
return ErrConnClosed
}
c.manager.wait()
// Hang up.
// TODO(light): add timeout to write.
ctx := context.Background()
Expand Down

0 comments on commit f6d1181

Please sign in to comment.