mgo

Merge lp:~rogpeppe/mgo/eliminate-acquire-race into lp:mgo/v2

Proposed by Roger Peppe
Status: Rejected
Rejected by: Gustavo Niemeyer
Proposed branch: lp:~rogpeppe/mgo/eliminate-acquire-race
Merge into: lp:mgo/v2
Diff against target: 23 lines (+6/-4)
1 file modified
session.go (+6/-4)
To merge this branch: bzr merge lp:~rogpeppe/mgo/eliminate-acquire-race
Reviewer Review Type Date Requested Status
Gustavo Niemeyer Pending
Review via email: mp+157374@code.launchpad.net

Description of the change

To post a comment you must log in.
Revision history for this message
Roger Peppe (rogpeppe) wrote :

Reviewers: mp+157374_code.launchpad.net,

Message:
Please take a look.

Description:
mgo: fix possible race

https://code.launchpad.net/~rogpeppe/mgo/eliminate-acquire-race/+merge/157374

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8367044/

Affected files:
   A [revision details]
   M session.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: session.go
=== modified file 'session.go'
--- session.go 2013-03-27 00:33:51 +0000
+++ session.go 2013-04-05 14:05:14 +0000
@@ -2894,14 +2894,16 @@
   // Read-only lock to check for previously reserved socket.
   s.m.RLock()
   if s.masterSocket != nil {
+ sock := s.masterSocket
+ sock.Acquire()
    s.m.RUnlock()
- s.masterSocket.Acquire()
- return s.masterSocket, nil
+ return sock, nil
   }
   if s.slaveSocket != nil && s.slaveOk && slaveOk {
+ sock := s.slaveSocket
+ sock.Acquire()
    s.m.RUnlock()
- s.slaveSocket.Acquire()
- return s.slaveSocket, nil
+ return sock, nil
   }
   s.m.RUnlock()

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Thanks, but I already have a local change for this after we talked
yesterday. I haven't pushed yet because it's shelved in the middle of
other major changes that I'm hoping to release today or over the
weekend.

https://codereview.appspot.com/8367044/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

ok, i thought you might, but proposed just in case.

On 5 April 2013 15:18, <email address hidden> wrote:
> Thanks, but I already have a local change for this after we talked
> yesterday. I haven't pushed yet because it's shelved in the middle of
> other major changes that I'm hoping to release today or over the
> weekend.
>
> https://codereview.appspot.com/8367044/

Unmerged revisions

197. By Roger Peppe

eliminate the possibility of a race in session.acquireSocket

196. By Roger Peppe

bson: make NewObjectId thread safe

R=dfc, niemeyer
CC=
https://codereview.appspot.com/8306043

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'session.go'
2--- session.go 2013-03-27 00:33:51 +0000
3+++ session.go 2013-04-05 14:07:48 +0000
4@@ -2894,14 +2894,16 @@
5 // Read-only lock to check for previously reserved socket.
6 s.m.RLock()
7 if s.masterSocket != nil {
8+ sock := s.masterSocket
9+ sock.Acquire()
10 s.m.RUnlock()
11- s.masterSocket.Acquire()
12- return s.masterSocket, nil
13+ return sock, nil
14 }
15 if s.slaveSocket != nil && s.slaveOk && slaveOk {
16+ sock := s.slaveSocket
17+ sock.Acquire()
18 s.m.RUnlock()
19- s.slaveSocket.Acquire()
20- return s.slaveSocket, nil
21+ return sock, nil
22 }
23 s.m.RUnlock()
24

Subscribers

People subscribed via source and target branches

to all changes: