Fix Family::get_or_create

[nox]

The method should not overwrite an existing key after it obtains the write lock.

Consider the following scenario with Family<LabelSet, Gauge> used by threads A and B:

1. A and B both call family.get_or_insert(&label_set)
2. A and B both acquire read lock and finds no key
3. A gets write lock and inserts a new gauge with value 0
4. A increments returned gauge to 1
5. B gets write lock and inserts a new gauge with value 0
6. B increments returned gauge to 1
7. A decrements gauge back to 0
8. B decrements gauge which now overflows to u64::MAX

[mxinden]

Oh silly me.

Great catch. Thanks for the patch.

Can you include a changelog entry?

Out of curiosity, did you hit this race condition, or found it while reading?

[nox]

Out of curiosity, did you hit this race condition, or found it while reading?

We discovered it the hard way with gauges reporting increments of 18.4 quintillions after making the switch in 3 repos.

[nox]

I'll add a changelog entry. Can you release 0.18.1 with that fix?

[mxinden]

Out of curiosity, did you hit this race condition, or found it while reading?

We discovered it the hard way with gauges reporting increments of 18.4 quintillions after making the switch in 3 repos.

Sorry to hear that. My bad. Thanks for debugging.

Do you have some time to tackle the second race condition as well? If not, I will give it a shot tomorrow.

I'll add a changelog entry. Can you release 0.18.1 with that fix?

Yes, I will do that as soon as we have both race conditions fixed. See below.

[mxinden]

With Family::remove there is actually a second race condition.

In the scenarios where we have two threads:

1. A: Call Family::get_or_create with a new label set.
   1. Acquiring the read lock. Nothing there.
   2. Acquiring the write lock. Inserting the new metric.
   3. Being preempted by the OS.
2. B: Call Family::remove with the above label set.
3. A: Acquiring the read lock for the second (last) time, .expect()ing the previously inserted metric to be there.

This can be prevented by leveraging RwLockWriteGuard::downgrade of the previously acquired write lock instead of doing acquiring another read lock via self.metrics.read.

Does the above make sense?

[nox]

It makes sense yes. The patch now uses RwLockWriteGuard::downgrade.

[mxinden]

Released with #103 as v0.18.1. Merged into master via #104.

I am closing here.

Again, thanks @nox for debugging and thanks for providing a fix. Great work.