Skip to content
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

runtime: TestCallbackInAnotherThread failures with timeout waiting for thread to exit #62206

Closed
gopherbot opened this issue Aug 22, 2023 · 6 comments
Labels
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. OS-Windows Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@gopherbot
Copy link
Contributor

gopherbot commented Aug 22, 2023

#!watchflakes
default <- pkg == "runtime" && test == "TestCallbackInAnotherThread" && `timeout waiting for thread to exit`

Issue created automatically to collect these failures.

Example (log):

--- FAIL: TestCallbackInAnotherThread (0.14s)
    syscall_windows_test.go:272: timeout waiting for thread to exit

watchflakes

@gopherbot gopherbot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 22, 2023
@gopherbot
Copy link
Contributor Author

Found new dashboard test flakes for:

#!watchflakes
default <- pkg == "runtime" && test == "TestCallbackInAnotherThread"
2023-08-22 02:48 windows-amd64-2016 go@c140105e runtime.TestCallbackInAnotherThread (log)
--- FAIL: TestCallbackInAnotherThread (0.14s)
    syscall_windows_test.go:272: timeout waiting for thread to exit

watchflakes

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

bcmills commented Aug 22, 2023

It appears that this test is using a hard-coded 100ms timeout:
https://cs.opensource.google/go/go/+/master:src/runtime/syscall_windows_test.go;l=268;drc=dd78bc0564509f8c410d6467f6c4c74646bfb224

It looks like this test was added in https://go.dev/cl/114755 for #17997. Is there some reason it needs to set a timeout on this call at all?

(CC @golang/windows @golang/runtime)

@bcmills bcmills changed the title runtime: TestCallbackInAnotherThread failures runtime: TestCallbackInAnotherThread failures with timeout waiting for thread to exit Aug 23, 2023
@qmuntal
Copy link
Member

qmuntal commented Aug 24, 2023

Is there some reason it needs to set a timeout on this call at all?

I don't see the value on defining a WaitForSingleObject timeout instead of relying on the built-in testing timeout, IMO we could remove it and close this issue.

@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. labels Aug 24, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 24, 2023
@mknyszek mknyszek added this to the Backlog milestone Aug 30, 2023
@mknyszek
Copy link
Contributor

@qmuntal Are you planning to send a patch for this? If not, let us know! Thanks.

@qmuntal
Copy link
Member

qmuntal commented Aug 30, 2023

@qmuntal Are you planning to send a patch for this? If not, let us know! Thanks.

I will, thanks for the remaninder.

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/524695 mentions this issue: runtime: remove unnecessary timeout in TestCallbackInAnotherThread

@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Aug 31, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Aug 31, 2023
@golang golang locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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. OS-Windows Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
Archived in project
Development

No branches or pull requests

5 participants