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 <josh@tailscale.com>
This commit is contained in:
Josh Bleecher Snyder 2021-05-07 12:50:40 -07:00 committed by Josh Bleecher Snyder
parent a72fb7ac0b
commit 8d2a90529e
1 changed files with 1 additions and 3 deletions

View File

@ -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()
}