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

ensure filehandles are closed #173

Merged
merged 2 commits into from
Jan 16, 2021
Merged

Conversation

docwhat
Copy link
Contributor

@docwhat docwhat commented Jan 15, 2021

@docwhat
Copy link
Contributor Author

docwhat commented Jan 15, 2021

I can't reproduce this locally... I think it is a legit error:

=== FAIL: internal/filewatcher TestHasGoFiles/none (0.00s)
    --- FAIL: TestHasGoFiles/none (0.00s)
panic: sync C:\Users\circleci\AppData\Local\Temp\TestHasGoFiles-none-706743447: The handle is invalid. [recovered]
	panic: sync C:\Users\circleci\AppData\Local\Temp\TestHasGoFiles-none-706743447: The handle is invalid.

goroutine 16 [running]:
testing.tRunner.func1.1(0xc62da0, 0xc00005ee40)
	c:/go/src/testing/testing.go:1072 +0x310
testing.tRunner.func1(0xc00016c300)
	c:/go/src/testing/testing.go:1075 +0x43a
panic(0xc62da0, 0xc00005ee40)
	c:/go/src/runtime/panic.go:969 +0x1c7
gotest.tools/gotestsum/internal/filewatcher.hasGoFiles(0xc000012460, 0x42, 0xc00000a800)
	C:/Users/circleci/project/internal/filewatcher/watch.go:167 +0x493
gotest.tools/gotestsum/internal/filewatcher.TestHasGoFiles.func1(0xc00016c300)
	C:/Users/circleci/project/internal/filewatcher/watch_test.go:88 +0x115
testing.tRunner(0xc00016c300, 0xca6a70)
	c:/go/src/testing/testing.go:1123 +0xef
created by testing.(*T).Run
	c:/go/src/testing/testing.go:1168 +0x2b3

=== FAIL: internal/filewatcher TestHasGoFiles (0.00s)

@docwhat
Copy link
Contributor Author

docwhat commented Jan 15, 2021

I'd be okay with disabling at least this lint message due to this reasoning.

internal/filewatcher/watch.go:160:16: Error return value of `fh.Close` is not checked (errcheck)
	defer fh.Close()

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for tracking down the problem and opening this PR! You are absolutely correct this missing fh.Close() is a problem. golangci-lint has linters for catching missing close on http response bodies, and sql rows, but apparently not for file handlers unfortunately.

That article is a great reminder about how to safely handle writes. Thankfully in this case we are only doing reads (https://golang.org/pkg/os/#Open, "Open opens the named file for reading"). I believe that explains the panic we see from windows. I guess on linux it is not an error to try and sync with a read-only handle, but on windows it is.

Since this is read-only we can remove the fh.Sync(), there isn't anything to sync.

I agree we can also ignore the lint error about the ignored return, we don't care about errors in this case because it is read-only.

I've added some suggested edits which should resolve the build errors. I'll apply those and merge once CI is happy.

internal/filewatcher/watch.go Outdated Show resolved Hide resolved
internal/filewatcher/watch.go Outdated Show resolved Hide resolved
internal/filewatcher/watch.go Outdated Show resolved Hide resolved
internal/filewatcher/watch.go Outdated Show resolved Hide resolved
@dnephin
Copy link
Member

dnephin commented Jan 16, 2021

LGTM! CI is happy with those changes!

@dnephin dnephin merged commit 22e9314 into gotestyourself:main Jan 16, 2021
@docwhat docwhat deleted the pr/close-watch branch January 19, 2021 19:51
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.

2 participants