Skip to content

Use atomics in stats#403

Closed
clecat wants to merge 1 commit intomirage:mainfrom
clecat:main
Closed

Use atomics in stats#403
clecat wants to merge 1 commit intomirage:mainfrom
clecat:main

Conversation

@clecat
Copy link
Contributor

@clecat clecat commented Sep 18, 2025

Index should not be expected to be thread safe, however it's stats might be acceded from several domains.
This PR simply replaces the global stats with atomics, better than nothing and avoids basic race conditions.

@lyrm
Copy link
Contributor

lyrm commented Sep 18, 2025

Hi! Could you instead protect the concurrent calls to Index.Stats with a mutex where you use it?

The use of Atomic prevents data races (so tsan will be happier) but does not prevent race conditions, meaning that it does not prevent the result from being wrong due to some bad interleavings. In this case, you could have bad statistics and think it comes from your use of index, whereas it is because Stats is not working well and because there are no data races anymore, you will have no way to realise it. So pretty terrible.

So, to clarify, I suggest using a mutex in irmin without changing the implementation of Index.

@clecat
Copy link
Contributor Author

clecat commented Sep 18, 2025

I believe both need to be done, atomics to avoid data races, expose them as non atomics values and use them with mutexes when necessary in irmin.

@lyrm
Copy link
Contributor

lyrm commented Sep 18, 2025

Mutexes are enough to protect from data races and make it easier to avoid race conditions (the same way kcas does). Also, the issue with doing a partial fix here is that it may seem thread-safe, while it is not meant to be used without mutexes.

To simplify the change in Irmin, you could have a decorator module Stats in irmin that only adds the lock/unlock to all the functions of Index.Stats. This is not the only solution or the best one (using kcas could be a possibility, for example), but it will likely be the easiest while avoiding the pitfall of race conditions (and data races).

@clecat
Copy link
Contributor Author

clecat commented Sep 19, 2025

As you prefer, I'll look into that

@clecat clecat closed this Sep 19, 2025
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.

2 participants