From a471b8242b5f8a505f9e0c18c5871fb7e18ff2d9 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Fri, 15 Nov 2019 12:03:18 +0100 Subject: [PATCH] p2p/simulations: fix lock copying (found by go vet) --- p2p/simulations/events.go | 3 +- p2p/simulations/http_test.go | 16 +++------- p2p/simulations/network.go | 33 ++++++++++--------- p2p/simulations/network_test.go | 56 +++++++++++---------------------- 4 files changed, 41 insertions(+), 67 deletions(-) diff --git a/p2p/simulations/events.go b/p2p/simulations/events.go index 984c2e088f11..d0d03794edf7 100644 --- a/p2p/simulations/events.go +++ b/p2p/simulations/events.go @@ -73,8 +73,7 @@ func NewEvent(v interface{}) *Event { switch v := v.(type) { case *Node: event.Type = EventTypeNode - node := *v - event.Node = &node + event.Node = v.copy() case *Conn: event.Type = EventTypeConn conn := *v diff --git a/p2p/simulations/http_test.go b/p2p/simulations/http_test.go index 84f6ce2a517c..e88999f48b8c 100644 --- a/p2p/simulations/http_test.go +++ b/p2p/simulations/http_test.go @@ -420,16 +420,8 @@ type expectEvents struct { } func (t *expectEvents) nodeEvent(id string, up bool) *Event { - node := Node{ - Config: &adapters.NodeConfig{ - ID: enode.HexID(id), - }, - up: up, - } - return &Event{ - Type: EventTypeNode, - Node: &node, - } + config := &adapters.NodeConfig{ID: enode.HexID(id)} + return &Event{Type: EventTypeNode, Node: newNode(nil, config, up)} } func (t *expectEvents) connEvent(one, other string, up bool) *Event { @@ -450,7 +442,7 @@ loop: for { select { case event := <-t.events: - t.Logf("received %s event: %s", event.Type, event) + t.Logf("received %s event: %v", event.Type, event) if event.Type != EventTypeMsg || event.Msg.Received { continue loop @@ -486,7 +478,7 @@ func (t *expectEvents) expect(events ...*Event) { for { select { case event := <-t.events: - t.Logf("received %s event: %s", event.Type, event) + t.Logf("received %s event: %v", event.Type, event) expected := events[i] if event.Type != expected.Type { diff --git a/p2p/simulations/network.go b/p2p/simulations/network.go index 58fd9a28b0fe..ef5451e77ec4 100644 --- a/p2p/simulations/network.go +++ b/p2p/simulations/network.go @@ -119,10 +119,7 @@ func (net *Network) NewNodeWithConfig(conf *adapters.NodeConfig) (*Node, error) if err != nil { return nil, err } - node := &Node{ - Node: adapterNode, - Config: conf, - } + node := newNode(adapterNode, conf, false) log.Trace("Node created", "id", conf.ID) nodeIndex := len(net.Nodes) @@ -448,7 +445,7 @@ func (net *Network) GetNodeIDs(excludeIDs ...enode.ID) []enode.ID { } func (net *Network) getNodeIDs(excludeIDs []enode.ID) []enode.ID { - // Get all curent nodeIDs + // Get all current nodeIDs nodeIDs := make([]enode.ID, 0, len(net.nodeMap)) for id := range net.nodeMap { nodeIDs = append(nodeIDs, id) @@ -735,7 +732,16 @@ type Node struct { // up tracks whether or not the node is running up bool - upMu sync.RWMutex + upMu *sync.RWMutex +} + +func newNode(an adapters.Node, ac *adapters.NodeConfig, up bool) *Node { + return &Node{Node: an, Config: ac, up: up, upMu: new(sync.RWMutex)} +} + +func (n *Node) copy() *Node { + configCpy := *n.Config + return newNode(n.Node, &configCpy, n.Up()) } // Up returns whether the node is currently up (online) @@ -787,22 +793,19 @@ func (n *Node) MarshalJSON() ([]byte, error) { }) } -// UnmarshalJSON implements json.Unmarshaler interface so that we don't lose -// Node.up status. IMPORTANT: The implementation is incomplete; we lose -// p2p.NodeInfo. +// UnmarshalJSON implements json.Unmarshaler interface so that we don't lose Node.up +// status. IMPORTANT: The implementation is incomplete; we lose p2p.NodeInfo. func (n *Node) UnmarshalJSON(raw []byte) error { // TODO: How should we turn back NodeInfo into n.Node? // Ticket: https://github.com/ethersphere/go-ethereum/issues/1177 - node := struct { + var node struct { Config *adapters.NodeConfig `json:"config,omitempty"` Up bool `json:"up"` - }{} + } if err := json.Unmarshal(raw, &node); err != nil { return err } - - n.SetUp(node.Up) - n.Config = node.Config + *n = *newNode(nil, node.Config, node.Up) return nil } @@ -899,7 +902,7 @@ func (net *Network) snapshot(addServices []string, removeServices []string) (*Sn Nodes: make([]NodeSnapshot, len(net.Nodes)), } for i, node := range net.Nodes { - snap.Nodes[i] = NodeSnapshot{Node: *node} + snap.Nodes[i] = NodeSnapshot{Node: *node.copy()} if !node.Up() { continue } diff --git a/p2p/simulations/network_test.go b/p2p/simulations/network_test.go index f504b9a698b7..d0cb592036cb 100644 --- a/p2p/simulations/network_test.go +++ b/p2p/simulations/network_test.go @@ -758,27 +758,22 @@ func benchmarkMinimalServiceTmp(b *testing.B) { } func TestNode_UnmarshalJSON(t *testing.T) { - t.Run( - "test unmarshal of Node up field", - func(t *testing.T) { - runNodeUnmarshalJSON(t, casesNodeUnmarshalJSONUpField()) - }, - ) - t.Run( - "test unmarshal of Node Config field", - func(t *testing.T) { - runNodeUnmarshalJSON(t, casesNodeUnmarshalJSONConfigField()) - }, - ) + t.Run("up_field", func(t *testing.T) { + runNodeUnmarshalJSON(t, casesNodeUnmarshalJSONUpField()) + }) + t.Run("config_field", func(t *testing.T) { + runNodeUnmarshalJSON(t, casesNodeUnmarshalJSONConfigField()) + }) } func runNodeUnmarshalJSON(t *testing.T, tests []nodeUnmarshalTestCase) { t.Helper() for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var got Node - if err := got.UnmarshalJSON([]byte(tt.marshaled)); err != nil { + var got *Node + if err := json.Unmarshal([]byte(tt.marshaled), &got); err != nil { expectErrorMessageToContain(t, err, tt.wantErr) + got = nil } expectNodeEquality(t, got, tt.want) }) @@ -788,7 +783,7 @@ func runNodeUnmarshalJSON(t *testing.T, tests []nodeUnmarshalTestCase) { type nodeUnmarshalTestCase struct { name string marshaled string - want Node + want *Node wantErr string } @@ -812,7 +807,7 @@ func expectErrorMessageToContain(t *testing.T, got error, want string) { } } -func expectNodeEquality(t *testing.T, got Node, want Node) { +func expectNodeEquality(t *testing.T, got, want *Node) { t.Helper() if !reflect.DeepEqual(got, want) { t.Errorf("Node.UnmarshalJSON() = %v, want %v", got, want) @@ -824,23 +819,17 @@ func casesNodeUnmarshalJSONUpField() []nodeUnmarshalTestCase { { name: "empty json", marshaled: "{}", - want: Node{ - up: false, - }, + want: newNode(nil, nil, false), }, { name: "a stopped node", marshaled: "{\"up\": false}", - want: Node{ - up: false, - }, + want: newNode(nil, nil, false), }, { name: "a running node", marshaled: "{\"up\": true}", - want: Node{ - up: true, - }, + want: newNode(nil, nil, true), }, { name: "invalid JSON value on valid key", @@ -867,26 +856,17 @@ func casesNodeUnmarshalJSONConfigField() []nodeUnmarshalTestCase { { name: "Config field is omitted", marshaled: "{}", - want: Node{ - Config: nil, - }, + want: newNode(nil, nil, false), }, { name: "Config field is nil", - marshaled: "{\"config\": nil}", - want: Node{ - Config: nil, - }, + marshaled: "{\"config\": null}", + want: newNode(nil, nil, false), }, { name: "a non default Config field", marshaled: "{\"config\":{\"name\":\"node_ecdd0\",\"port\":44665}}", - want: Node{ - Config: &adapters.NodeConfig{ - Name: "node_ecdd0", - Port: 44665, - }, - }, + want: newNode(nil, &adapters.NodeConfig{Name: "node_ecdd0", Port: 44665}, false), }, } }