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

race condition on table.file #557

Closed
domcyrus opened this issue Dec 1, 2022 · 1 comment
Closed

race condition on table.file #557

domcyrus opened this issue Dec 1, 2022 · 1 comment
Labels
bug Something isn't working. ready-for-release Feature is implemented and available for testing in dev branch. It will be included in the next rele

Comments

@domcyrus
Copy link
Contributor

domcyrus commented Dec 1, 2022

Describe the bug

There is a race condition in table.file module. The problem is related to the comment on https://github.com/foxcpp/maddy/blob/master/internal/table/file_test.go#L114

	// This delay is somehow important. Not sure why.
	time.Sleep(500 * time.Millisecond)

Steps to reproduce

go test -run TestFileReload -count 1000

Eventually you will hit the issue and the unit test will fail with:

--- FAIL: TestFileReload (3.51s)
    file_test.go:134: New m were not loaded

The problem is that in unix/linux OS the code which creates the file is not atomic and therefore there is a race condition that although the code is stat'ing the file it will actually read an empty file instead of the content cat: dog.

Now the question is how to fix this. It is trivial to fix the unit test in a way that we use the standard procedure to first write a tempfile with the content and then do an atomic rename of the file. This would fix the unit test BUT I think we should actually also fix the real issue in the code e.g. in that it should not only check the last modified date but also whether content is already written to the file.

  • maddy version: master
@domcyrus domcyrus added the bug Something isn't working. label Dec 1, 2022
@domcyrus
Copy link
Contributor Author

domcyrus commented Dec 1, 2022

After the discussion on IRC we will skipping changes if the last time since modified is less than the reload interval. FYI @foxcpp

domcyrus added a commit to domcyrus/maddy that referenced this issue Dec 3, 2022
foxcpp added a commit that referenced this issue Dec 5, 2022
@foxcpp foxcpp added the ready-for-release Feature is implemented and available for testing in dev branch. It will be included in the next rele label Jan 8, 2023
@foxcpp foxcpp closed this as completed Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. ready-for-release Feature is implemented and available for testing in dev branch. It will be included in the next rele
Projects
None yet
Development

No branches or pull requests

2 participants