New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ServerStats should be protected against its mutex #122

Closed
skypexu opened this Issue Mar 25, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@skypexu
Copy link

skypexu commented Mar 25, 2015

diff --git a/Server/ServerStats.cc b/Server/ServerStats.cc
index 001ef3a..d4ce30b 100644
--- a/Server/ServerStats.cc
+++ b/Server/ServerStats.cc
@@ -30,7 +30,7 @@ namespace Server {

 ServerStats::Lock::Lock(ServerStats& wrapper)
     : wrapper(wrapper)
-    , lockGuard()
+    , lockGuard(wrapper.mutex)
 {
 }

@ongardie ongardie closed this in 8145125 Mar 26, 2015

ongardie added a commit that referenced this issue Mar 26, 2015

Switch from std::unique_lock to std::lock_guard where possible
In general, we should prefer std::lock_guard to std::unique_lock. When
simple scope-based locking is needed, this is more obvious to readers
and less error-prone (see #122).
@ongardie

This comment has been minimized.

Copy link
Member

ongardie commented Mar 26, 2015

Nice one, thanks @skypexu! I just pushed the change to master.

It's too bad there's a default constructor on std::unique_lock that does nothing yet doesn't look at all suspicious. This is probably a good argument for using std::lock_guard instead wherever possible. The second commit here makes those changes throughout LogCabin.

Out of curiosity:

  • How did you find this? Manual inspection or automated tools?
  • Did this issue cause problems for you in testing?
@skypexu

This comment has been minimized.

Copy link

skypexu commented Mar 27, 2015

Hi,

I am learning raft, so I was reading logcabin code, and manually found
the bug.

Regards,

On 2015年03月27日 02:49, Diego Ongaro wrote:

Nice one, thanks @skypexu https://github.com/skypexu! I just pushed
the change to master.

It's too bad there's a default constructor on std::unique_lock that
does nothing yet doesn't look at all suspicious. This is probably a
good argument for using std::lock_guard instead wherever possible. The
second commit here makes those changes throughout LogCabin.

Out of curiosity:

  • How did you find this? Manual inspection or automated tools?
  • Did this issue cause problems for you in testing?


Reply to this email directly or view it on GitHub
#122 (comment).

@ongardie

This comment has been minimized.

Copy link
Member

ongardie commented Mar 27, 2015

@skypexu: I see, well thanks for finding it :)

@skypexu

This comment has been minimized.

Copy link

skypexu commented Apr 2, 2015

Hi,

Does logcabin support pipeline AppendEntries? I am reading code and can
not find it.
It seems the code uses a single thread for each peer and rpcCall blocks
until peer returned
response.

Regards,
Xu

@ongardie

This comment has been minimized.

Copy link
Member

ongardie commented Apr 2, 2015

@skypexu, see #97: some suboptimal code for it exists but has not been merged.

@ongardie ongardie added the bug label Jul 13, 2015

nhardt pushed a commit to nhardt/logcabin that referenced this issue Aug 21, 2015

Fix missing locking in ServerStats::Lock
Thanks to @skypexu for the patch.

Close logcabin#122: ServerStats should be protected against its mutex

nhardt pushed a commit to nhardt/logcabin that referenced this issue Aug 21, 2015

Switch from std::unique_lock to std::lock_guard where possible
In general, we should prefer std::lock_guard to std::unique_lock. When
simple scope-based locking is needed, this is more obvious to readers
and less error-prone (see logcabin#122).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment