I think there is still a race here where we drain the channel on line 395, then send() selects on line 469 and hits the default case, then we write the copy to the channel on line 401. You end up not writing the data.

Given that we're in session shutdown or write timeout, do we want to even bother copying the body? Like, we're going to return an error, so I don't think the sender will have the expectation that the data was actually written.

Good catch! This subtle error was introduced by the change in a9a7d9e, the default case was not supposed to be present any longer.

The reason we care about copying the body is that we have no "cancel" mechanism for writes where the header has already been written, so we allow the message to pass as-is if it reaches this junction. If we don't write the body a future write could fail due to incomplete packet (header, missing body). The idea is to keep the behavior as close to the original implementation as possible, whilst doing it safely.

Presumably this chan has to be allocated on the heap, which will make more work for the garbage collector. If we're concerned about that kind of hit, we could add a mutex to sendReady that needs to be held while accessing the body in send() and bodyCopy.

It's a good suggestion and I did actually consider a mutex, but a channel seemed more appropriate in this case. It'd be interesting to do some benchmarks, but according to go build -gcflags '-m -m' it doesn't escape to the heap. But maybe I'm using/interpreting that wrong.

I think it's accurate to say that bodyC itself doesn't need to escape to the heap, since it is just copied to the sendReady. But, the chan type is itself a reference, and the thing it points to, hchan, is allocated on the heap: https://levelup.gitconnected.com/how-does-golang-channel-works-6d66acd54753

Yeah that's true. I wasn't sure how much of a penalty the channels are though, so I made a quick benchmark to see how much of a difference there is. And it's not huge, but we can save 1 alloc and a little bit of memory by using a mutex here. Big picture it's quite negligible but since I already wrote the code, let's go with it 😄.

Benchmark results (do note that wait group and groutine skews the raw results here):

Channels:

❯ go test -benchmem -run='^$' -bench '^BenchmarkSession_send' -count=3 -benchtime=2s
goos: linux
goarch: amd64
pkg: github.com/hashicorp/yamux
cpu: Intel Core Processor (Broadwell, IBRS)
BenchmarkSession_send-chan-6   	   13564	    157409 ns/op	   31389 B/op	     501 allocs/op
BenchmarkSession_send-chan-6   	   14619	    166961 ns/op	   31396 B/op	     501 allocs/op
BenchmarkSession_send-chan-6   	   15681	    164911 ns/op	   31391 B/op	     501 allocs/op

If we change chan to mutex, and sendReady to a pointer chan *sendReady (to avoid mutex copy):

BenchmarkSession_send-mu-6   	   13770	    153617 ns/op	   25756 B/op	     400 allocs/op
BenchmarkSession_send-mu-6   	   16140	    166674 ns/op	   25764 B/op	     401 allocs/op
BenchmarkSession_send-mu-6   	   16030	    151941 ns/op	   25755 B/op	     400 allocs/op

And if we restore chan *sendReady => chan sendReady, and change the mutex to a pointer (mu *sync.Mutex), we save a bit more:

BenchmarkSession_send-mu2-6   	   15700	    149387 ns/op	   20130 B/op	     400 allocs/op
BenchmarkSession_send-mu2-6   	   16130	    140559 ns/op	   20129 B/op	     400 allocs/op
BenchmarkSession_send-mu2-6   	   15886	    138668 ns/op	   20128 B/op	     400 allocs/op

For reference, my quick and dirty benchmark:

type fakeConn struct {
	io.ReadCloser
	io.Writer
}

func BenchmarkSession_send(b *testing.B) {
	conn := &fakeConn{
		Writer: io.Discard,
	}
	s := &Session{
		logger: log.New(os.Stderr, "test", log.LstdFlags),
		conn:   conn,
		sendCh: make(chan sendReady, 64),
		config: &Config{
			ConnectionWriteTimeout: 5 * time.Second,
		},
	}
	go s.send()

	hdr := header(make([]byte, headerSize))
	body := make([]byte, 1024)
	var wg sync.WaitGroup
	n := 100

	for i := 0; i < b.N; i++ {
		wg.Add(n)
		for i := 0; i < n; i++ {
			errc := make(chan error, 1)
			go func() {
				s.waitForSendErr(hdr, body, errc)
				wg.Done()
			}()
		}
		wg.Wait()
	}
}

FYI: Not using WaitGroup and running a single s.waitForSendErr synchronously produces 4 allocs for chan, 3 allocs for mutex.

5619618