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.

Sign up for GitHub

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

Jump to bottom

Watch.Add improvements (avoid race, fix consistency, reduce garbage) #189

Merged
merged 3 commits into from
Mar 21, 2017
Merged

Watch.Add improvements (avoid race, fix consistency, reduce garbage) #189

merged 3 commits into from
Mar 21, 2017

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Nov 14, 2016

This PR fixes two potential problems in Watch.Add in inotify.go:

  • Race when Add() is called concurrently.
  • Incorrect flags stored when InotifyAddWatch fails.

In addition, it re-uses the existing watch entry when modifying an existing watch.

The individual commits contain more details.

@twpayne
hold mutex for longer to avoid race condition
81cea74 
If Watcher.Add is called with the same name from multiple goroutines and
name is not already in the watches map then the last call to complete
wins and the watch created by the first call to complete is lost.

This commit holds the mutex for the full duration of the update to the
watches map.
@twpayne
avoid clobbering existing flags if inotify_add_watch fails
ddad047 
Previously, the watch entry is the watches map was updated before the
call to InotifyAddWatch. If InotifyAddWatch fails then this left the
watch entry in an inconsistent state.
@twpayne twpayne mentioned this pull request Nov 14, 2016
Closed
@twpayne
Copy link
Contributor Author

twpayne commented Dec 5, 2016

Any chance of getting this reviewed?

@twpayne
Copy link
Contributor Author

twpayne commented Dec 20, 2016

Gentle ping :)

@nathany
Copy link
Contributor

nathany commented Dec 21, 2016

Sorry for the delay. I'll try to carve out some time over the Christmas holiday to work on fsnotify.

@twpayne
Copy link
Contributor Author

twpayne commented Jan 15, 2017

Gentle ping :)

@ppknap
Copy link
Contributor

ppknap commented Jan 15, 2017 •
edited

@twpayne - I'm not a maintainer of this package but these changes LGTM.

Race when Add() is called concurrently.

👍, moreover, it fixes potential deadlock when Add(..) and Remove(..) are called concurrently with the same path...

Incorrect flags stored when InotifyAddWatch fails.

(Unfortunately) this is not entirely true, because the current fsnotify logic always sets all inotify events to be watched (agnosticEvents). This also implies that unix.IN_MASK_ADD is not needed at all. However, this is a different story...

Overall, this PR is an improvement of current logic.

@twpayne
Copy link
Contributor Author

twpayne commented Jan 16, 2017

Many thanks for the review!

@twpayne
Copy link
Contributor Author

twpayne commented Feb 15, 2017 •
edited

@nathany Any chance of getting this merged?

@nathany
Copy link
Contributor

nathany commented Feb 18, 2017

@twpayne Thanks and apologies for the delay. @ppknap Thanks also for reviewing.

One consideration is the performance of wide vs. narrow mutexes:
https://talks.golang.org/2017/state-of-go.slide#25

But I think this is a good change, and we can come back to performance considerations later.

I'll take a closer look and merge + prepare a release in the next few days. Thanks!

@nathany nathany added the linux label Feb 18, 2017
@twpayne
Copy link
Contributor Author

twpayne commented Feb 18, 2017

Thanks!

Note that in this case, the "wide vs. narrow mutexes" fix is about correctness, not performance. See the commit message of 81cea74 for more details.

@twpayne
Copy link
Contributor Author

twpayne commented Mar 14, 2017

Four months after submission, this PR is approved, passed all checks, and ready to merge. Is this gonna happen? Is fsnotify abandonware?

@nathany
Copy link
Contributor

nathany commented Mar 14, 2017

@twpayne It's only abandonware if nobody steps in to replace me.

https://groups.google.com/forum/#!topic/golang-nuts/Ix4sg_gDLNE
see #183

@twpayne
Copy link
Contributor Author

twpayne commented Mar 21, 2017

Sorry, I am not able to step in to replace you. I have sufficient maintenance burden with my existing open source projects.

Is there any reason for not merging this PR? You only have to press the merge button.

@ppknap
Copy link
Contributor

ppknap commented Mar 21, 2017

@twpayne @nathany I'm pretty sure this code won't break anything system dependent.

@ppknap ppknap merged commit ff7bc41 into fsnotify:master Mar 21, 2017
@twpayne twpayne deleted the inotify-race branch March 21, 2017 12:57
@nathany
Copy link
Contributor

nathany commented Mar 21, 2017

@twpayne No worries.

You are right, for a Linux-only change, CI is enough and hitting merge is fine. Thanks @ppknap for reviewing and merging.

@twpayne Did you happen to sign the Google CLA at some point? That process still needs to be automated. #193.

There are a few more steps to do a release which should probably be documented somewhere. Updating the AUTHORS file, CHANGELOG, and Releases after tagging a version. Creating an issue around that: #201.

@twpayne
Copy link
Contributor Author

twpayne commented Mar 21, 2017

Yes, I've signed the Google CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppknap ppknap ppknap approved these changes

Assignees
No one assigned
Labels
linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@twpayne @nathany @ppknap