Skip to content

Fix #28: ReadFrom no longer holds the lock while blocked in rd.Read - #29

Merged
smallnest merged 1 commit into
masterfrom
fix/issue-28-readfrom-holds-lock
Sep 8, 2026
Merged

Fix #28: ReadFrom no longer holds the lock while blocked in rd.Read#29
smallnest merged 1 commit into
masterfrom
fix/issue-28-readfrom-holds-lock

Conversation

@smallnest

Copy link
Copy Markdown
Owner

Problem (#28)

ReadFrom acquired r.mu once and held it via defer for the entire loop, calling rd.Read while the mutex was still held. When the source is idle (e.g. an idle TCP connection), rd.Read blocks with the lock held, freezing the whole buffer — Read, Peek, Length, Free and IsEmpty all block until bytes arrive.

This is reachable without ever naming the method: io.Copy(rb, rd) dispatches to ReadFrom through io.ReaderFrom.

Fix

Release r.mu for the duration of rd.Read while preserving ReadFrom's zero-copy contract (it reads directly into r.buf).

  • writeMu serializes the write side. ReadFrom holds it for its whole duration and reserves the contiguous free window beyond the write pointer, releasing r.mu during rd.Read. The reserved window is disjoint from the committed [r, w) region that readers and Peek/Length touch, so the read/inspection side stays responsive and correct while a read is in flight.
  • Write entry points take writeMu (Write/WriteByte/TryWrite/TryWriteByte), lock order always writeMu → r.mu, so no concurrent writer scribbles into the reserved window or moves r.w out from under the snapshot. Readers and Reset never take writeMu, so they remain independent.
  • Generation snapshot. The generation is captured before releasing r.mu; if Reset() lands while blocked in rd.Read, the stale bytes are dropped and the loop recomputes against the fresh state.

Tests

readfrom_idle_test.go:

  • TestReadFromIdleSourceDoesNotBlockInspectors — regression test: inspectors return promptly while ReadFrom is parked in an idle read. Deadlocks before this fix.
  • TestReadFromConcurrentWithInspectorsAndDrain — slow multi-chunk source vs. concurrent inspect + drain.
  • TestReadFromConcurrentWithReset — hammers Reset() mid-copy to exercise the generation snapshot/discard path.

All green under go test -race -count=1 ./...; gofmt -l . empty; go vet ./... clean.

Note

This PR also bumps go.mod to 1.26 and applies matching modernizations (WaitGroup.Go, testing.B.Loop, interface{}any). If forcing a Go version bump on downstream users is undesirable, the fix itself is independent of the bump and can be rebased onto go 1.19 with the regression test rewritten in the older idioms — happy to split it out.

Closes #28

ReadFrom took r.mu once and held it via defer for the whole loop, calling
rd.Read while the mutex was still held. Whenever the source was idle (e.g. an
idle TCP connection), rd.Read blocked with the lock held and froze the entire
buffer: Read, Peek, Length, Free and IsEmpty all blocked until bytes arrived.
This is reachable without naming the method, since io.Copy(rb, rd) dispatches to
ReadFrom via io.ReaderFrom.

Fix, preserving the zero-copy contract (ReadFrom reads directly into r.buf):

- Add writeMu to serialize the write side. ReadFrom holds it for its whole
  duration and reserves the contiguous free window beyond the write pointer,
  releasing r.mu during rd.Read. Because the reserved window is disjoint from
  the committed [r, w) region, readers and inspectors stay responsive and
  correct while a read is in flight.
- Guard Write/WriteByte/TryWrite/TryWriteByte with writeMu (lock order is always
  writeMu -> r.mu) so no concurrent writer scribbles into the reserved window or
  moves r.w out from under the snapshot.
- Snapshot the generation before releasing r.mu; if Reset() lands while blocked
  in rd.Read, the stale bytes are dropped and the loop recomputes against the
  fresh state.

Add readfrom_idle_test.go: a regression test proving inspectors return promptly
while ReadFrom is parked in an idle read (deadlocks before this fix), plus
concurrent slow-source and Reset stress tests under -race.

Also bump go.mod to 1.26 and modernize (WaitGroup.Go, testing.B.Loop, any).
@smallnest
smallnest merged commit 6cba436 into master Sep 8, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ReadFrom holds the mutex while blocked in rd.Read, blocking every other method whenever the source is idle

1 participant