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

fix: Use SyscallConn for isTTY which is safe during file close #167

Merged
merged 2 commits into from
May 16, 2023

Conversation

mafredri
Copy link
Member

We now use (*os.File).SyscallConn in isTTY check which is safe during file close.

Added a test that produces the error and the change fixes it.

=== RUN   TestEntry
=== PAUSE TestEntry
=== CONT  TestEntry
=== RUN   TestEntry/isTTY_during_file_close
=== PAUSE TestEntry/isTTY_during_file_close
=== CONT  TestEntry/isTTY_during_file_close
==================
WARNING: DATA RACE
Read at 0x00c000094430 by goroutine 9:
  os.(*File).Fd()
      /usr/local/go/src/os/file_unix.go:89 +0xa4
  cdr.dev/slog/internal/entryhuman.isTTY()
      /home/mafredri/src/coder/slog/internal/entryhuman/entry.go:185 +0x98
  cdr.dev/slog/internal/entryhuman.shouldColor()
      /home/mafredri/src/coder/slog/internal/entryhuman/entry.go:189 +0x36
  cdr.dev/slog/internal/entryhuman.c()
      /home/mafredri/src/coder/slog/internal/entryhuman/entry.go:40 +0xc5
  cdr.dev/slog/internal/entryhuman.Fmt()
      /home/mafredri/src/coder/slog/internal/entryhuman/entry.go:54 +0x84
  cdr.dev/slog/internal/entryhuman_test.TestEntry.func8.1()
      /home/mafredri/src/coder/slog/internal/entryhuman/entry_test.go:106 +0x150

Previous write at 0x00c000094430 by goroutine 10:
  internal/poll.(*FD).destroy()
      /usr/local/go/src/internal/poll/fd_unix.go:86 +0xcb
  internal/poll.(*FD).decref()
      /usr/local/go/src/internal/poll/fd_mutex.go:213 +0x3e
  internal/poll.(*FD).Close()
      /usr/local/go/src/internal/poll/fd_unix.go:107 +0x84
  os.(*file).close()
      /usr/local/go/src/os/file_unix.go:262 +0x124
  os.(*File).Close()
      /usr/local/go/src/os/file_posix.go:25 +0x4b
  cdr.dev/slog/internal/entryhuman_test.TestEntry.func8.2()
      /home/mafredri/src/coder/slog/internal/entryhuman/entry_test.go:115 +0x4c

Goroutine 9 (running) created at:
  cdr.dev/slog/internal/entryhuman_test.TestEntry.func8()
      /home/mafredri/src/coder/slog/internal/entryhuman/entry_test.go:105 +0x1c5
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1629 +0x47

Goroutine 10 (finished) created at:
  cdr.dev/slog/internal/entryhuman_test.TestEntry.func8()
      /home/mafredri/src/coder/slog/internal/entryhuman/entry_test.go:114 +0x295
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1629 +0x47
==================
    testing.go:1446: race detected during execution of test
--- FAIL: TestEntry (0.00s)
    --- FAIL: TestEntry/isTTY_during_file_close (0.00s)
FAIL
FAIL	cdr.dev/slog/internal/entryhuman	0.032s
FAIL

Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

Fascinating didn't know this was a thing. Nice!

@coveralls
Copy link

Pull Request Test Coverage Report for Build dccb6b21208886bec0e1fc63d71d4ed0bb1df864-PR-167

  • 14 of 16 (87.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 96.986%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/entryhuman/entry.go 14 16 87.5%
Totals Coverage Status
Change from base Build fd83a3567145b5f6fb420d911620938d9214ede7: -0.2%
Covered Lines: 740
Relevant Lines: 763

💛 - Coveralls

@ammario ammario merged commit 2202bcf into main May 16, 2023
@ammario ammario deleted the mafredri/safer-istty branch May 16, 2023 20:40
@ammario ammario mentioned this pull request May 30, 2023
ammario added a commit that referenced this pull request May 30, 2023
coadler added a commit that referenced this pull request Aug 17, 2023
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.

6 participants