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

database/sql: TestQueryContext flakes with "failed to close rows" #54555

Open
heschi opened this issue Aug 19, 2022 · 2 comments
Open

database/sql: TestQueryContext flakes with "failed to close rows" #54555

heschi opened this issue Aug 19, 2022 · 2 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@heschi
Copy link
Contributor

heschi commented Aug 19, 2022

2022-08-08T14:12:03-fe406c8/windows-amd64-race
2022-08-08T14:11:47-ba89d59/windows-amd64-race

--- FAIL: TestQueryContext (359.09s)
    sql_test.go:410: failed to close rows
    sql_test.go:184: 1 connections still open after closing DB
FAIL

cc @bradfitz @kardianos @kevinburke

@heschi heschi added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 19, 2022
@heschi heschi added this to the Go1.20 milestone Aug 19, 2022
@kardianos
Copy link
Contributor

I've been unable to reproduce. Though I am not testing on windows.

In this (and other) tests, it waits for up to 5 seconds to close the row after it is canceled. So far I've been unable to reproduce the failure. But this type of failure could be from too much load and the goroutine that will close the row, to not get scheduled in that grace time.

We could remove the local timeout, but that doesn't seem ideal either. I'm unsure how to handle this.

@bcmills
Copy link
Contributor

bcmills commented Aug 23, 2022

But this type of failure could be from too much load and the goroutine that will close the row, to not get scheduled in that grace time.

Note that the test failure in the above snippet ran the test for nearly 6 minutes before failing. (As of CL 378395, the test should be much less sensitive to specific timing issues.)

If this sort of test fails, would a goroutine dump show what is holding the rows open? If so, perhaps have the test call debug.SetTraceback("all") and panic instead of calling t.Fatalf.

Or, would it be possible for the test to log more information about the lifecycle events for the connections that are still open?

@gopherbot gopherbot modified the milestones: Go1.20, Go1.21 Feb 1, 2023
@gopherbot gopherbot modified the milestones: Go1.21, Go1.22 Aug 8, 2023
@odeke-em odeke-em modified the milestones: Go1.22, Go1.23 Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants