Use read_lock

[ganmacs]

It's no use to use write lock here since these methods only read the value.

[coveralls]

Coverage remained the same at 100.0% when pulling dfd3c3b on ganmacs:use-read-lock into 3652cf7 on prometheus:master.

[coveralls]

Coverage remained the same at 100.0% when pulling dfd3c3b on ganmacs:use-read-lock into 3652cf7 on prometheus:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 23b2d04 on ganmacs:use-read-lock into 3652cf7 on prometheus:master.

[dmagliola]

Technically, yes.
What is the advantage of using the read lock, however, in this case?

From my point of view, this makes the code a bit more complicated, and the locking a bit harder to reason about. And the only advantage, if I understand correctly, would be if there were two threads reading values simultaneously, which would be a quite unusual use case.

[ganmacs]

would be if there were two threads reading values simultaneously, which would be a quite unusual use case.

I don't think so. For example puma. it supports multi-threads server. and this repository supports rack interface, right? it means prometheus/client_ruby runs on puma.
So, I think that two threads reading values simultaneously is easily occurred in such an environment.
besides: I thought the code very was confusing when I first saw. if you think read_lock is a bit complicated, why don't you use Monitor class, which provides the reentrant lock (not read/write lock), in this class?
When I read this code for the first time, I thought that this code has something special intention not to use read_lock where the code only reads the data.

But if you think this code makes the code complicated, feel free to close this PR 👍

[dmagliola]

Hi @ganmacs! Just a little update to say I haven't forgotten about this, and i'm planning to look into it today or tomorrow.

[splattael]

Suggested change

	it '#set an #get' do
	it '#set and #get' do

[dmagliola]

Ok, having looked into this a bit more...
The only reason I was using the Read-Write Lock is that it was a reentrant lock.
I didn't know that Monitor was reentrant!

This is an awesome bit of information, as I had added the concurrent-ruby gem just to get the ReadWriteLock, which I needed just to have a reentrant lock. And I'm really keen on having as few dependencies as possible on gems.
So with this new bit of information, I propose to change approach here, remove the concurrent-ruby gem, and simply use Monitor.
This removes the confusion of getting a write lock when trying to read a value, which I agree, is confusing.

What do you think?

And the only advantage, if I understand correctly, would be if there were two threads reading values simultaneously, which would be a quite unusual use case.

To clarify this comment, which was not very well worded...

While we do explicitly support multi-threaded scenarios, my expectation when using the client is that values are very frequently incremented, but very rarely read.
Pretty much the only case where values are read is when getting scraped. This happens infrequently, and is already mutexed by locking the files being read.
The metrics do allow reading single values, but I'm expecting that to be a very rare scenario, and I can't personally come up with any cases where one would want to do that. This is probably a failure of imagination, but I still wouldn't expect it to happen often enough that the lock will add a lot of contention.

So, what I should've said is "The only advantage is when reading single values from multiple threads, and I expect reading single values would be a quite unusual case."

[dmagliola]

@ganmacs This is my proposal instead: #137
Thoughts?

[ganmacs]

Looks good to me. I'll close this PR. Thank you!