search

hack-pad / hackpad

The in-browser IDE for Go
https://hackpad.org
Apache License 2.0
531 stars 37 forks source link

Race condition in pub sub #1

Closed hherman1 closed 3 years ago

hherman1 commented 3 years ago

https://github.com/JohnStarich/go-wasm/blob/fd0edf41a665e400fef3db6616772439ce4b9738/internal/pubsub/pubsub.go#L40

Go routines A and B call emit(). Both see that visited == false under read lock. A acquires write lock and processes. B acquired right lock and double processes, even though A marked the node as visited. The fix is to check if visited again after write lock is acquired

JohnStarich commented 3 years ago

Hey @hherman1, good eye. Do you have a test case where the behavior changes?

If I'm reading it right, looks like the behavior should be the same in both cases despite the race condition. Feel free to open a PR to make the code more obviously correct, of course.

hherman1 commented 3 years ago

Not sure if there are any consequences -- not familiar with the context of where this is used, just noticed the bug and wanted to let you know about it. feel free to ignore/close if it seems unimportant to you

JohnStarich commented 3 years ago

All good! I really appreciate your looking out for bugs ❤️

I wrote a few tests and didn't observe a behavior change within pubsub, so I'll close for now. If anyone wants to open a PR to make it more clearly correct, that'd be fine too 👍