Skip to content

Commit 210ed3c

Browse files
committed
quic: establish a "happened-before" relationship between stream write and read
This ensures that our race detector will not get tripped off in tests where we do something like the following: 1. Client writes to a shared variable. 2. Client sends a request to a server. 3. Server handler reads the shared variable. We are technically over-synchronizing right now by always using the address of quicSync when calling runtime.RaceAcquire and runtime.RaceReleaseMerge. However, that is fine for now since this mostly matters just for testing, and creating a mapping of stream ID to addressess are rather unclean. For golang/go#78737 Change-Id: Id65964ad3aa0b2e998e8faf11bfda3c76a6a6964 Reviewed-on: https://go-review.googlesource.com/c/net/+/777940 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: Nicholas Husin <husin@google.com>
1 parent ad8140e commit 210ed3c

4 files changed

Lines changed: 85 additions & 2 deletions

File tree

quic/race_disabled.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// Copyright 2026 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
//go:build !race
6+
7+
package quic
8+
9+
func raceAcquire() {}
10+
func raceReleaseMerge() {}

quic/race_enabled.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// Copyright 2026 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
//go:build race
6+
7+
package quic
8+
9+
import (
10+
"runtime"
11+
"unsafe"
12+
)
13+
14+
// Our synchronization here is rather coarse-grained since we always use the
15+
// address of quicSync. As such, it will not differentiate between, for
16+
// example, reads and writes to different QUIC streams.
17+
// However, this is consistent with our synchronization for net/http, and
18+
// mostly only matters for testing.
19+
var quicSync uint64
20+
21+
func raceAcquire() {
22+
runtime.RaceAcquire(unsafe.Pointer(&quicSync))
23+
}
24+
25+
func raceReleaseMerge() {
26+
runtime.RaceReleaseMerge(unsafe.Pointer(&quicSync))
27+
}

quic/stream.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -246,8 +246,8 @@ func (s *Stream) Read(b []byte) (n int, err error) {
246246
fastPath := false
247247
s.inbufmu.Lock()
248248
if len(s.inbuf) > s.inbufoff {
249-
// Fast path: If s.inbuf contains unread bytes, return them immediately
250-
// without taking a lock.
249+
// Fast path: If s.inbuf contains unread bytes, return them
250+
// immediately.
251251
n = copy(b, s.inbuf[s.inbufoff:])
252252
s.inbufoff += n
253253
fastPath = true
@@ -302,6 +302,7 @@ func (s *Stream) Read(b []byte) (n int, err error) {
302302
bytesRead = int64(len(b))
303303
start := s.in.start
304304
end := start + int64(len(b))
305+
raceAcquire()
305306
s.in.copy(start, b)
306307
s.in.discardBefore(end)
307308
if end == s.insize {
@@ -462,6 +463,7 @@ func (s *Stream) Write(b []byte) (n int, err error) {
462463
}
463464
s.outbufmu.Unlock()
464465
}
466+
raceReleaseMerge()
465467
s.outUnlock()
466468
return n, nil
467469
}

quic/stream_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1623,6 +1623,50 @@ func TestStreamInbufReadRace(t *testing.T) {
16231623
})
16241624
}
16251625

1626+
// TestStreamWriteHappensBeforeRead verifies that a write to a stream
1627+
// establishes a happens-before relationship with a read from the stream, as
1628+
// seen by the race detector.
1629+
func TestStreamWriteHappensBeforeRead(t *testing.T) {
1630+
// Use real UDP sockets via newLocalConnPair to avoid the synchronization
1631+
// inherent in the channel-based testConn infrastructure.
1632+
cli, srv := newLocalConnPair(t, &Config{}, &Config{})
1633+
var written bool
1634+
var wg sync.WaitGroup
1635+
defer wg.Wait()
1636+
wg.Go(func() {
1637+
s, err := srv.AcceptStream(t.Context())
1638+
if err != nil {
1639+
t.Fatal(err)
1640+
}
1641+
defer s.Close()
1642+
1643+
if _, err := io.ReadFull(s, make([]byte, 1)); err != nil {
1644+
t.Fatal(err)
1645+
}
1646+
1647+
// Access shared variable after reading from stream. If the stream read
1648+
// doesn't establish happens-before with the stream write, this will be
1649+
// flagged as a data race.
1650+
if !written {
1651+
t.Error("written is false, want true")
1652+
}
1653+
})
1654+
1655+
s, err := cli.NewSendOnlyStream(t.Context())
1656+
if err != nil {
1657+
t.Fatal(err)
1658+
}
1659+
defer s.Close()
1660+
1661+
written = true // Write shared variable
1662+
// Write to stream. This should happen-before the read of x in the
1663+
// goroutine.
1664+
_, err = s.Write([]byte{1})
1665+
if err != nil {
1666+
t.Fatal(err)
1667+
}
1668+
}
1669+
16261670
type streamSide string
16271671

16281672
const (

0 commit comments

Comments
 (0)