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

Prevent data race when closing active file #60

Closed

Conversation

mlaventure
Copy link
Contributor

The race usually happens when closeHandle() and prepareIo() are called
concurrently; the former tries to set closing to true the latter tries
to read its value.

In order to avoid this issue, we added a lock around the variable.

Signed-off-by: Kenfe-Mickael Laventure [email protected]

@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@mlaventure
Copy link
Contributor Author

Just realized that both #31 and #59 are proposed fix for this 😅

Feel free to close if any of them is merged.

@darstahl
Copy link
Contributor

I also left this same comment in #59, but:

We already have an "atomicBool" type in this file used for a different bool with a similar issue. I'd prefer to use that instead of adding a Mutex.

type atomicBool int32

func (b *atomicBool) isSet() bool { return atomic.LoadInt32((*int32)(b)) != 0 }
func (b *atomicBool) setFalse()   { atomic.StoreInt32((*int32)(b), 0) }
func (b *atomicBool) setTrue()    { atomic.StoreInt32((*int32)(b), 1) }

See deadlineHandler.timedout for example usage.

I've confirmed that this makes the race detector happy.

@mlaventure mlaventure force-pushed the fix-win32file-data-race branch from 22928a7 to 6cd24b4 Compare July 24, 2017 23:12
This should prevent a race that usually happens when `closeHandle()` and
`prepareIo()` are called concurrently; the former tries to set `closing` to
`true` the latter tries to read its value.

Signed-off-by: Kenfe-Mickael Laventure <[email protected]>
@mlaventure mlaventure force-pushed the fix-win32file-data-race branch from 6cd24b4 to 9bdb224 Compare July 24, 2017 23:14
@mlaventure
Copy link
Contributor Author

@darrenstahlmsft updated, used a compare-and-swap for setting closing to true though.

@darstahl
Copy link
Contributor

#59 was merged, so I'm closing this. Thanks though!

@darstahl darstahl closed this Jul 25, 2017
@mlaventure
Copy link
Contributor Author

mlaventure commented Jul 25, 2017 via email

@mlaventure mlaventure deleted the fix-win32file-data-race branch July 25, 2017 13:10
@darstahl
Copy link
Contributor

I merged that on my phone last night, taking v0.4.4 right now.

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