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

Added tests to show race conditions #4

Merged
merged 3 commits into from
Jul 13, 2020
Merged

Added tests to show race conditions #4

merged 3 commits into from
Jul 13, 2020

Conversation

pstibrany
Copy link
Member

@pstibrany pstibrany commented Nov 19, 2019

Added two tests to show race conditions in Tell and Size methods. (#3)

Looking at Go API docs, I don't see any guarantees about os.File being safe for concurrent access from different goroutines, so unless we want to have mutex around all file usages, safest way may be to remove (!!) these two methods completely.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Copy link
Member Author

(Oh yes, while at it, I've converted this to use go modules)

@slim-bean
Copy link
Collaborator

We need the Tell method to update the positions file in promtail, and the Size method sort of helps with an alert we have for making sure we are tailing files.

Adding a mutex around only the places where we set the file addresses the race from the test perspective. Although to your point we could be calling a method on that as it's changed (didn't mutex the other methods) Though I think this is probably ok? Maybe we get the size from a different file but the way we use this I think that will end up being not important (we poll those functions every 10 seconds or so)

We could also mutex the other places to be thorough, I opened #5 which gets the tests to pass. WDYT @pstibrany ?

@pstibrany pstibrany marked this pull request as ready for review January 27, 2020 14:10
@pstibrany
Copy link
Member Author

@slim-bean Should we merge this as well (if it passes now)?

@pstibrany
Copy link
Member Author

(ok, this also does go mod conversion, so maybe not. perhaps needs a closer look...)

@slim-bean
Copy link
Collaborator

I'm find with the go mod conversion, we can merge this too!

@slim-bean slim-bean merged commit b926940 into master Jul 13, 2020
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