Skip to content

runtime/internal/wasitest: TestTCPEcho is racy #61820

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.

Sign up for GitHub

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

Jump to bottom
Closed
bcmills opened this issue Aug 7, 2023 · 5 comments
Closed

runtime/internal/wasitest: TestTCPEcho is racy #61820

bcmills opened this issue Aug 7, 2023 · 5 comments
Labels
arch-wasm WebAssembly issues compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.22

Comments

@bcmills
Copy link
Contributor

bcmills commented Aug 7, 2023

TestTCPEcho has a logical race condition: it opens a listener on a local TCP port, closes that port, and then assumes that the port is available for reuse (and not, say, taken by some other test process that is running concurrently):
https://cs.opensource.google/go/go/+/master:src/runtime/internal/wasitest/tcpecho_test.go;l=33-38;drc=3687f77069a8f40a596be7bd848985990908f0d9

It should be rewritten to avoid that race if possible. If avoiding the race is impossible, the test should be skipped by default (and require an explicit flag or environment variable to be set in order to run).

(attn @golang/wasm @johanbrandhorst @chriso)
@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. arch-wasm WebAssembly issues labels Aug 7, 2023
@bcmills bcmills added this to the Go1.22 milestone Aug 7, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Aug 7, 2023

The test also contains a hard-coded timeout of 2.5s, which should be eliminated:
https://cs.opensource.google/go/go/+/master:src/runtime/internal/wasitest/tcpecho_test.go;l=67-73;drc=3687f77069a8f40a596be7bd848985990908f0d9

A heavily loaded CI machine may suffer arbitrarily long delays. In general, I find that it is best to just let the test loop until it times out, at which point it will dump the running goroutines for debugging. If that doesn't produce the right information to diagnose a failure, the next best option is to use t.Deadline() and subtract some padding to account for logging and shutdown.

@bcmills bcmills added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 7, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Aug 7, 2023

@gopherbot, please backport to Go 1.21. This is a racy test, and a flaky failure was observed during the Go 1.21.0 release process.

@gopherbot gopherbot mentioned this issue Aug 7, 2023
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #61821 (for 1.21).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/519895 mentions this issue: runtime/internal/wasitest: skip racy TCP echo test

@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Aug 18, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/520955 mentions this issue: [release-branch.go1.21] runtime/internal/wasitest: skip racy TCP echo test

gopherbot pushed a commit that referenced this issue Aug 18, 2023
@chriso @gopherbot
[release-branch.go1.21] runtime/internal/wasitest: skip racy TCP echo…

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
70aa116 
… test

The wasip1 TCP echo test introduced in CL 493358 has a race
condition with port selection. The test runner probes for a free
port and then asks the WASM runtime to listen on the port, which
may be taken by another process in the interim.

Due to limitations with WASI preview 1, the guest is unable to
query the port it's listening on. The test cannot ask the WASM
runtime to listen on port 0 (choose a free port) since there's
currently no way for the test to query the selected port and
connect to it.

Given the race condition is unavoidable, this test is now disabled
by default and requires opt-in via an environment variable.

This commit also eliminates the hard-coded connection timeout.

Updates #61820.
Fixes #61821.

Change-Id: I375145c1a1d03ad45c44f528da3347397e6dcb01
Reviewed-on: https://go-review.googlesource.com/c/go/+/519895
Run-TryBot: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
(cherry picked from commit 795e779)
Reviewed-on: https://go-review.googlesource.com/c/go/+/520955
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
@golang golang locked and limited conversation to collaborators Aug 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
2 participants
@bcmills @gopherbot