Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings
/slogPublic

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ammario merged 2 commits intomainfrommafredri/safer-istty
May 16, 2023

Conversation

@mafredri
Copy link
Member

We now use(*os.File).SyscallConn inisTTY 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 RACERead 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 +0x150Previous 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 +0x4cGoroutine 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 +0x47Goroutine 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)FAILFAILcdr.dev/slog/internal/entryhuman0.032sFAIL

Copy link
Contributor

@nhooyrnhooyr 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 forBuild dccb6b21208886bec0e1fc63d71d4ed0bb1df864-PR-167

  • 14 of16(87.5%) changed or added relevant lines in1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to96.986%
Changes Missing CoverageCovered LinesChanged/Added Lines%
internal/entryhuman/entry.go141687.5%
TotalsCoverage Status
Change from baseBuild fd83a3567145b5f6fb420d911620938d9214ede7:-0.2%
Covered Lines:740
Relevant Lines:763
💛 -Coveralls

@ammarioammario merged commit2202bcf intomainMay 16, 2023
@ammarioammario deleted the mafredri/safer-istty branchMay 16, 2023 20:40
@ammarioammario mentioned this pull requestMay 30, 2023
ammario added a commit that referenced this pull requestMay 30, 2023
coadler added a commit that referenced this pull requestAug 17, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@kylecarbskylecarbskylecarbs approved these changes

@deansheatherdeansheatherdeansheather approved these changes

+1 more reviewer

@nhooyrnhooyrnhooyr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mafredri@coveralls@kylecarbs@nhooyr@deansheather@ammario
[8]ページ先頭
©2009-2025 Movatter.jp