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

chore: fix race condition in watcher implementations #10

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

neelayu
Copy link

@neelayu neelayu commented Nov 16, 2022

There seems to be a race condition in fw.size when I ran the test.
I have used atomic.XXInt64 operations to ensure atomic read and writes.

Running tool: /usr/local/go/bin/go test -timeout 30s -run ^TestWatchNotify$ github.com/influxdata/tail/watch -race -v logtostderr

=== RUN   TestWatchNotify
=== RUN   TestWatchNotify/Test_watch_inotify
Modified
==================
WARNING: DATA RACE
Write at 0x00c000012028 by goroutine 9:
  github.com/influxdata/tail/watch.(*InotifyFileWatcher).ChangeEvents()
      /Users/neelay.upadhyaya/workspace/tail/watch/inotify.go:75 +0x22a
  github.com/influxdata/tail/watch.watchFile()
      /Users/neelay.upadhyaya/workspace/tail/watch/watch_test.go:136 +0x214
  github.com/influxdata/tail/watch.TestWatchNotify.func1.1()
      /Users/neelay.upadhyaya/workspace/tail/watch/watch_test.go:49 +0xa4

Previous read at 0x00c000012028 by goroutine 12:
  github.com/influxdata/tail/watch.(*InotifyFileWatcher).ChangeEvents.func1()
      /Users/neelay.upadhyaya/workspace/tail/watch/inotify.go:135 +0x529

Logs are truncated, but they are also present in other places.

@neelayu
Copy link
Author

neelayu commented Nov 16, 2022

@powersj can you check out this PR? Thanks!

Copy link

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thanks - are you planning on making additional changes in a future PR?

@powersj powersj requested a review from srebhan November 16, 2022 14:27
@neelayu
Copy link
Author

neelayu commented Nov 16, 2022

Good guess indeed!. I do have a change planned. Hopefully its okay to contribute 😅
I would be adding a watcher event support for file create as well, when the watcher is on a directory. Do you see this as a fruitful change? I intend to use it in telegraf

@powersj
Copy link

powersj commented Nov 17, 2022

Do you see this as a fruitful change?

+1 on fixing tests, and we can always review your PR, could you file an issue to layout what you are doing?

To be honest, I don't think this project has had much attention, it doesn't look like CI is even set up anymore, so I can look into that as well.

@neelayu
Copy link
Author

neelayu commented Nov 21, 2022

Thanks. I have created an issue. You can take a look at it.

@srebhan srebhan self-assigned this Nov 30, 2022
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for this fix @neelayu!

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.

3 participants