Add doc about mem.*Unsafe consuming the input buffer

[Groxx]

I've been reading the new buffer-pool-related code due to recent OOM problems with other libraries misusing sync.Pool, and the varying behavior between Buffer and SliceBuffer had me confused for quite a while.

Buffer mutates itself when read, and implicitly calls Free when fully consumed:

grpc-go/mem/buffers.go

Lines 194 to 200 in 5edab9e

	n := copy(buf, b.data)
	if n == len(b.data) {
	b.Free()
	return n, nil
	}
	b.data = b.data[n:]

While SliceBuffer returns a sub-slice but does not modify itself:

grpc-go/mem/buffers.go

Lines 263 to 267 in 5edab9e

	n := copy(buf, s)
	if n == len(s) {
	return n, nil
	}
	return n, s[n:]

The only way these *Unsafe funcs can be used correctly is by replacing the input-buffer with the result-buffer(s), as the current code does:

grpc-go/internal/transport/transport.go

Line 139 in 5edab9e

n, r.last = mem.ReadUnsafe(header, r.last)

... which seems worth documenting since it feels like a huge footgun otherwise: you only trigger the self-mutating-and-freeing behavior for relatively large data, which is less common in tests.

Granted, this isn't a technical barrier at all, but the behavior differences between implementations mislead me for a while and this "must not use input Buffer" requirement doesn't seem obviously-necessary to me when reading the code. Might as well save future-people that effort.

---

On a related note, Buffer feels... a bit concerningly strange in regards to concurrency, and I didn't see it called out in the PR that introduced it so I think it might not have been noticed.

E.g. Buffers are not concurrency-safe:

grpc-go/mem/buffers.go

Lines 39 to 41 in 5edab9e

	// Note that a Buffer is not safe for concurrent access and instead each
	// goroutine should use its own reference to the data, which can be acquired via
	// a call to Ref().

But it uses an atomic refcount:

grpc-go/mem/buffers.go

Lines 75 to 80 in 5edab9e

	type buffer struct {
	origData *[]byte
	data []byte
	refs *atomic.Int32
	pool BufferPool
	}

... but it doesn't use it in an atomically-safe way, as it sets the value to nil when freeing:

grpc-go/mem/buffers.go

Line 160 in 5edab9e

b.refs = nil

And if you include the fact that the ref came from a pool:

grpc-go/mem/buffers.go

Line 157 in 5edab9e

refObjectPool.Put(b.refs)

it seems like this code cannot possibly be correct:

• non-racing code is paying for atomics it does not need
• racing code can avoid some checks by using a type that is race-safe
  • this might not be reachable in practice though
• racing code can double-Put refcounts and buffers (with a racing Ref, Free, Ref, Free sequence)
• racing code can leak refcount changes across Buffer instances due to pooled reuse

So this is introducing difficult-to-investigate failure modes that non-atomic or non-pooled instances would not have. It seems like it'd be better to either just use an int field or not-reuse atomics (if the goal is to detect races).

Also I would be quite surprised if pooled *int values perform better than non-pooled, given the cheap initialization and sync.Pool's overhead, but I don't have any evidence either way.

Is there some benefit to this I'm missing? Maybe something about it can catch tricky races or misuse that would otherwise be missed? It seems misleading and dangerous to me otherwise.

[linux-foundation-easycla]

• ❌ - login: @Groxx / name: Steven L . The commit (0e114fe) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.01%. Comparing base (5edab9e) to head (0e114fe).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8209      +/-   ##
==========================================
- Coverage   82.17%   82.01%   -0.17%     
==========================================
  Files         410      410              
  Lines       40236    40236              
==========================================
- Hits        33065    32998      -67     
- Misses       5822     5873      +51     
- Partials     1349     1365      +16     

Files with missing lines	Coverage Δ
mem/buffers.go	88.18% <ø> (ø)

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[eshitachandwani]

@Groxx Can you please sign the CLA ?

[eshitachandwani]

@Groxx , the PR looks good to me , but for the other part , can you open a issue and we can have a discussion there.
Also adding Purnesh for a review.