From 8d2a90529ef45d83af2926d3ffadca351d8e87b3 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Fri, 7 May 2021 12:50:40 -0700 Subject: [PATCH] wgengine/bench: hold lock in TrafficGen.GotPacket while calling first packet callback Without any synchronization here, the "first packet" callback can be delayed indefinitely, while other work continues. Since the callback starts the benchmark timer, this could skew results. Worse, if the benchmark manages to complete before the benchmark timer begins, it'll cause a data race with the benchmark shutdown performed by package testing. That is what is reported in #1881. This is a bit unfortunate, in that it means that users of TrafficGen have to be careful to keep this callback speedy and lightweight and to avoid deadlocks. Fixes #1881 Signed-off-by: Josh Bleecher Snyder --- wgengine/bench/trafficgen.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/wgengine/bench/trafficgen.go b/wgengine/bench/trafficgen.go index 081505440..f2314dc9b 100644 --- a/wgengine/bench/trafficgen.go +++ b/wgengine/bench/trafficgen.go @@ -180,6 +180,7 @@ func (t *TrafficGen) Generate(b []byte, ofs int) int { // GotPacket processes a packet that came back on the receive side. func (t *TrafficGen) GotPacket(b []byte, ofs int) { t.mu.Lock() + defer t.mu.Unlock() s := &t.cur seq := int64(binary.BigEndian.Uint64( @@ -203,9 +204,6 @@ func (t *TrafficGen) GotPacket(b []byte, ofs int) { f := t.onFirstPacket t.onFirstPacket = nil - - t.mu.Unlock() - if f != nil { f() }