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

mem/file.go - Fix some races in accessing fields of FileData #134

Merged
merged 1 commit into from
Oct 3, 2017

Conversation

corentone
Copy link
Contributor

@corentone corentone commented Aug 25, 2017

  • Splitting SetModeTime to avoid double locking
  • Adding locks all over the place.

Potential fix for #133

@CLAassistant
Copy link

CLAassistant commented Aug 25, 2017

CLA assistant check
All committers have signed the CLA.

mem/file.go Outdated
f.name = newname
}

func SetMode(f *FileData, mode os.FileMode) {
f.Lock()
defer f.Unlock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to use defer when the method is so short.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, it would be great if you added a test that demonstrates this problem.

@corentone
Copy link
Contributor Author

corentone commented Oct 1, 2017

@bep thanks for the review and sorry for the delay addressing them.
Let me know if you have any comment, I also added some UT for the funcs I changed which are kind of boringly verbose...

I also enabled --race by default, it will make the UT slightly longer but should allow us to force running the tests more safely.

Now I realize I should maybe have added it in the main afero_test.go. Thoughts?

* Splitting SetModeTime to avoid double locking
* Adding locks all over the place.
@mbertschler mbertschler merged commit 44971ef into spf13:master Oct 3, 2017
@mbertschler
Copy link
Collaborator

mbertschler commented Oct 3, 2017

I verified that the new tests fail with data races without this patch.

@mbertschler
Copy link
Collaborator

Fixes #133

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.

4 participants