mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-05 14:31:29 +02:00
a1415dbc05
* [client] Add tests for the ICEBind open and close races Running many embedded clients in one process intermittently wedges interface creation. A goroutine dump taken from 50 clients shows ten of them parked for seven minutes in Device.IpcSet, in closeBindLocked waiting on device.net.stopping.Wait, holding device.net while every other device goroutine queues behind it on Device.Up. Open writes s.closed and Close reads it with no synchronisation, and Close also closes s.closedChan without the mutex that Open swaps it under. Two Closes can both pass the check and close the same channel, and a Close racing an Open can mark the bind closed while a live channel and live receive functions remain, after which every later Close takes its early return and runs neither close(closedChan) nor StdNetBind.Close. The receive functions never stop, so stopping.Wait never returns. These tests do not fix that. The first pins the contract closeBindLocked depends on and passes today. The other two fail under -race, reporting the races at the three sites above, and pass again once closed and closedChan are guarded consistently. * [client] Release parked receivers so reopening a bind cannot stall receiveRelayed held closedChanMu for the whole of its blocking select, so a parked receiver kept the read lock indefinitely and Open could never take the write lock it needs to install a fresh closedChan. wireguard-go reaches Open from Device.IpcSet and Device.Up with device.net held, so the stall took the device lock with it: interface creation never finished, every other device goroutine queued behind Device.Up, and Engine.Start never returned. Callers now copy the channel under a short read lock and select on the copy. Copying alone would stand a new trap in the same place, because an Open that follows an Open leaves the previous generation parked on a channel no later Close can reach, so Open now closes the outgoing channel before swapping it. closed and closedChan are also updated together under that mutex. Read and written apart, Close could see a stale closed and skip both close(closedChan) and StdNetBind.Close, leaving every receive function running and wedging closeBindLocked on device.net.stopping.Wait, or two Close calls could pass the check together and close the same channel twice. TestICEBindOpenDoesNotBlockOnParkedReceiver fails without this change, without needing the race detector. The other three cover the surrounding contract and report the state races under -race. * [client] Make the bind lifecycle transition atomic and tighten its tests Review caught that the previous commit moved the torn transition rather than removing it. Open published the new generation before calling StdNetBind.Open, so an Open rejected because the bind was already open had already signalled the outgoing generation, and a Close arriving in that window could mark the bind closed while the same call went on to install live sockets. Every later Close then returned early and never shut them down. Open now calls StdNetBind.Open first, so a failure leaves the current generation untouched, and both Open and Close hold the lock across the whole transition. Ordering is safe: StdNetBind.Open reaches muUDPMux through createReceiverFn, and no path takes muUDPMux before closedChanMu. The tests were also weaker than they read. The stress test claimed to cover a stale channel but only ever raced two Closes, and the concurrency test left overlap to goroutine start order. Both now gate their goroutines on a common start, the stress test races an Open against the Closes, and both assert the surviving generation channel is actually closed. Waiting on receive functions to be entered replaces part of the sleep in the reopen probe, and teardown bounds its Close so a regression fails the assertion instead of hanging. Two of the four now fail without the fix and no race detector, the stress test by reproducing close of a closed channel at the Close early return. * [client] Fail the reopen probe when its teardown does not complete closeBounded swallowed its timeout and the cleanup discarded what receiversStopped returned, so the bounds added in the previous commit only stopped teardown hanging. A wedged Close or a parked receiver would have left the test green with a leaked goroutine, which is the failure this test exists to catch. closeBounded now reports whether Close returned, and cleanup fails the test on either bound.