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

use sync.map is better #1145

Merged
merged 2 commits into from Nov 4, 2021
Merged

use sync.map is better #1145

merged 2 commits into from Nov 4, 2021

Conversation

liu-song
Copy link
Contributor

@liu-song liu-song commented Nov 3, 2021

The same as the pull request of #1106 the fileslockmap read is more than write ,so sync.map will be better

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

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

These are not the same I'm afraid. Something that went wrong in the previous pull request as well and was fixed here: 6d4db9b

With your new code it is possible for two goroutines to reach the exists == false state at the same time.

On way to fix this is to use LoadOrStore instead of just Load.

@liu-song
Copy link
Contributor Author

liu-song commented Nov 4, 2021

Thanks for review !
LoadOrStore will be better, I hava changed .here is my points about race condition,I'm not sure it's right,in this scenario when two goroutines reach the exists == false state at the same time, both of goroutines will store the value of *sync.Mutex, because of the implementation of the sync.map.store,there will be only one goroutine store success at the same time .
Another scenario , when two goroutine both return sync.Mutex to
https://github.com/valyala/fasthttp/blob/master/fs.go#L1124 , because of the implementation of the sync.mutex, there will only one goroutine get the mutex ,so it is safe for file when the value is mutex ? Is it different when the value of sync.map is *tcpAddrEntry to cause race condition?

@erikdubbelboer
Copy link
Collaborator

If both goroutines reach exists == false then only one of the mutexes will be stored in the map. But both goroutines will continue to lock their own mutex and think they have exclusive access to the file while they don't.

The tcpAddrEntry case is slightly different because it also checks some properties on it (.pending). That's why there I fixed it with an atomic check. Because of the use of atomic there only one goroutine will end up storing it's entry in the map.

@erikdubbelboer erikdubbelboer merged commit d613502 into valyala:master Nov 4, 2021
@erikdubbelboer
Copy link
Collaborator

Thanks!

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.

None yet

2 participants