Make conn.Close() wait till shutdown is complete.

[alanconway]

This showed up with the improved logging: log messages for the Close frames of one
test were printed after another test had started.

Should be deadlock safe, since we don't call user callbacks in the mux or tx/rx goroutines.

Signed-off-by: Alan Conway aconway@redhat.com

[vcabbage]

conn.errMu is supposed to be held by conn.mux until the connection is closed and all other goroutines have exited. It doesn't return until conn.close runs. conn.close closes conn.done and waits for conn.txDone and conn.rxDone.

conn.Close waits for conn.errMu via conn.getErr.

Perhaps conn.Close -> conn.getErr raced with conn.mux and acquired conn.errMu first. It's nicer to pair them up but for correctness it seems c.errMu.Lock() needs to happen before the mux goroutine is started.

amqp/conn.go

Lines 339 to 342 in ae5d459

	// hold the errMu lock until error or done
	c.errMu.Lock()
	defer c.errMu.Unlock()
	defer c.close() // defer order is important. c.errMu unlock indicates that connection is finally complete

[alanconway]

I don't think you need the mutex at all (and I wouldn't lock/unlock a mutex in different functions)
All the error checks are already guarded by a <-done check, so if you just set the error before close(c.done) you can access safely using only the channels Patch coming...

[alanconway]

This version drops the mutex, fixes the out-of-order logging and passes go test -race.

[vcabbage]

I was only going to accept this because it should have been a small fix. While you may well be correct, I'm not accepting any refactors.