Skip to content

Commit

Permalink
p2p: Wrap conn.flags ops with atomic.Load/Store
Browse files Browse the repository at this point in the history
  • Loading branch information
shazow committed Jun 8, 2018
1 parent ce10c1e commit 07ef2cc
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 20 deletions.
26 changes: 12 additions & 14 deletions p2p/peer.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,10 @@ type PeerEvent struct {

// Peer represents a connected remote node.
type Peer struct {
rw *conn
isInbound bool // Cached from rw.flags to avoid a race condition
running map[string]*protoRW
log log.Logger
created mclock.AbsTime
rw *conn
running map[string]*protoRW
log log.Logger
created mclock.AbsTime

wg sync.WaitGroup
protoErr chan error
Expand Down Expand Up @@ -161,20 +160,19 @@ func (p *Peer) String() string {

// Inbound returns true if the peer is an inbound connection
func (p *Peer) Inbound() bool {
return p.isInbound
return p.rw.is(inboundConn)
}

func newPeer(conn *conn, protocols []Protocol) *Peer {
protomap := matchProtocols(protocols, conn.caps, conn)
p := &Peer{
rw: conn,
isInbound: conn.is(inboundConn),
running: protomap,
created: mclock.Now(),
disc: make(chan DiscReason),
protoErr: make(chan error, len(protomap)+1), // protocols + pingLoop
closed: make(chan struct{}),
log: log.New("id", conn.id, "conn", conn.flags),
rw: conn,
running: protomap,
created: mclock.Now(),
disc: make(chan DiscReason),
protoErr: make(chan error, len(protomap)+1), // protocols + pingLoop
closed: make(chan struct{}),
log: log.New("id", conn.id, "conn", conn.flags),
}
return p
}
Expand Down
20 changes: 16 additions & 4 deletions p2p/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"fmt"
"net"
"sync"
"sync/atomic"
"time"

"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -187,7 +188,7 @@ type peerDrop struct {
requested bool // true if signaled by the peer
}

type connFlag int

This comment has been minimized.

Copy link
@shazow

shazow Jun 8, 2018

Author Contributor

atomic doesn't have a load/store for plain int.

type connFlag int32

const (
dynDialedConn connFlag = 1 << iota
Expand Down Expand Up @@ -252,7 +253,18 @@ func (f connFlag) String() string {
}

func (c *conn) is(f connFlag) bool {
return c.flags&f != 0
flags := connFlag(atomic.LoadInt32((*int32)(&c.flags)))
return flags&f != 0
}

func (c *conn) set(f connFlag, val bool) {
flags := connFlag(atomic.LoadInt32((*int32)(&c.flags)))
if val {
flags |= f
} else {
flags &= ^f
}
atomic.StoreInt32((*int32)(&c.flags), int32(flags))

This comment has been minimized.

Copy link
@zsfelfoldi

zsfelfoldi Aug 6, 2018

Contributor

This is not atomic :) Please use CompareAndSwap instead.

}

// Peers returns all connected peers.
Expand Down Expand Up @@ -632,7 +644,7 @@ running:
trusted[n.ID] = true
// Mark any already-connected peer as trusted
if p, ok := peers[n.ID]; ok {
p.rw.flags |= trustedConn
p.rw.set(trustedConn, true)
}
case n := <-srv.removetrusted:
// This channel is used by RemoveTrustedPeer to remove an enode
Expand All @@ -641,7 +653,7 @@ running:
delete(trusted, n.ID)
// Unmark any already-connected peer as trusted
if p, ok := peers[n.ID]; ok {
p.rw.flags &= ^trustedConn
p.rw.set(trustedConn, false)
}
case op := <-srv.peerOp:
// This channel is used by Peers and PeerCount.
Expand Down
2 changes: 0 additions & 2 deletions p2p/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,12 +189,10 @@ func TestServerDial(t *testing.T) {
}
done <- true
}()

// Trigger potential race conditions
peer = srv.Peers()[0]
_ = peer.Inbound()
_ = peer.Info()

<-done
case <-time.After(1 * time.Second):
t.Error("server did not launch peer within one second")
Expand Down

0 comments on commit 07ef2cc

Please sign in to comment.