StatsManager is not thread safe

[cangfengzhs]

Describe the bug (required)

sm.stats_.emplace(
      counterName,
      std::make_pair(std::make_unique<std::mutex>(),
                     std::make_unique<StatsType>(
                         60,
                         std::initializer_list<StatsType::Duration>(
                             {seconds(5), seconds(60), seconds(600), seconds(3600)}))));

stats_ is a std::unordered_map. And some time will read/write without lock

[Shylock-Hg]

It's protected by nameMapLock_.

[cangfengzhs]

No, nameMapLock_ protect another map. You can find some other places where stat_ is used without lock.

[Shylock-Hg]

No, nameMapLock_ protect another map. You can find some other places where stat_ is used without lock.

It protects stats_ too, add/delete counter will lock nameMapLock_ firstly.

[cangfengzhs]

But reading stat_ doesn't with Lock. If one thread read stat_'s element while another thread insert/erase some element, this may be lead to structure modify of unordered_map. Reading thread will access some released memory.

[Shylock-Hg]

But reading stat_ doesn't with Lock. If one thread read stat_'s element while another thread insert/erase some element, this may be lead to structure modify of unordered_map. Reading thread will access some released memory.

Yes, read need protection too.

[cangfengzhs]

Yes, read need protection too.

And I think this part requires some complex design rather than simply using read-write locks. The performance of the lock is very poor, and the metric cannot affect the performance of the system.

[Shylock-Hg]

Yes, read need protection too.

And I think this part requires some complex design rather than simply using read-write locks. The performance of the lock is very poor, and the metric cannot affect the performance of the system.

Agree, in my mind, static lifetime metric is enough.