Skip to content

restore devtunnel test #3050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 22, 2022
Merged

restore devtunnel test #3050

merged 3 commits into from
Jul 22, 2022

Conversation

spikecurtis
Copy link
Contributor

@spikecurtis spikecurtis commented Jul 19, 2022

Restores devtunnel test with a fake, local server.

Fixes #2335

Originally opened with flaky segfault behavior, but now fixed. See comments for diagnosis and eventual fix.

Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
@spikecurtis spikecurtis requested review from coadler and kylecarbs July 19, 2022 22:40
@spikecurtis
Copy link
Contributor Author

As an aside, I do still think it's worth open sourcing the tunnel server, but I decided not to try to use it for testing the client because it depends on DNS for proxying.

@spikecurtis
Copy link
Contributor Author

Ok, I've chased down the immediate problem, but I'm not clear how to fix it.

When you close a Device, this nils out the netstack.netTun.dispatcher that peer.RoutineSequentialReceiver() goroutine uses to communicate. There appears to be some sort of race condition where the goroutine then attempts to access this memory and segfaults.

@spikecurtis
Copy link
Contributor Author

Ok, some more information: we can avoid this segfault by calling device.RemoveAllPeers() before closing. This waits on the peers being stopped, and avoids the race that leads to segfault.

This should fixup the devtunnel, but the bug is still lurking in wireguard-go which is a tailscale dependency. Depending on their usage we could still be subject to these segfaults. I'll chase the wireguard maintainers about accepting a fix, or fixing it themselves. It's all via email, so no GitHub issue to track.

@spikecurtis spikecurtis force-pushed the spike/2335_devtunnel_test branch from 4c93b19 to 890623d Compare July 21, 2022 03:08
@spikecurtis spikecurtis force-pushed the spike/2335_devtunnel_test branch from 890623d to 35c6117 Compare July 21, 2022 03:18
@spikecurtis spikecurtis changed the title Draft: restore devtunnel test restore devtunnel test Jul 21, 2022
@spikecurtis spikecurtis requested review from a team and removed request for coadler and kylecarbs July 21, 2022 17:05
@spikecurtis spikecurtis merged commit fa4361d into main Jul 22, 2022
@spikecurtis spikecurtis deleted the spike/2335_devtunnel_test branch July 22, 2022 15:26
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.

test flake: coderd/devtunnel TestTunnel
2 participants