Skip to content

Use atomics in stats#403

Closed
clecat wants to merge 1 commit into
mirage:mainfrom
clecat:main
Closed

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

Conversation

@clecat

@clecat clecat commented Sep 18, 2025

Copy link
Copy Markdown
Contributor

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

lyrm commented Sep 18, 2025

Copy link
Copy Markdown
Contributor

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

clecat commented Sep 18, 2025

Copy link
Copy Markdown
Contributor Author

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

lyrm commented Sep 18, 2025

Copy link
Copy Markdown
Contributor

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

clecat commented Sep 19, 2025

Copy link
Copy Markdown
Contributor Author

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