Skip to content
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.

Sign up for GitHub

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

swarm/network/simulation: fix New function for-loop scope #18161

Merged
merged 5 commits into from
Nov 26, 2018
Merged

swarm/network/simulation: fix New function for-loop scope #18161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
06e9562
swarm/network: fix logs
nonsense Nov 21, 2018
f0da48c
swarm/network/simulation: improve New function comment
janos Nov 22, 2018
ae922ff
swarm/network/simulation: fix multiple services issue
janos Nov 22, 2018
f4958c0
swamr/network/simulation: simplify noopService2 in tests
janos Nov 23, 2018
ea62076
Merge remote-tracking branch 'origin/fix-logs' into simulation-new-co…
janos Nov 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions swarm/network/simulation/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,41 @@ func TestAddNodeWithService(t *testing.T) {
}
}

func TestAddNodeMultipleServices(t *testing.T) {
sim := New(map[string]ServiceFunc{
"noop1": noopServiceFunc,
"noop2": noopService2Func,
})
defer sim.Close()

id, err := sim.AddNode()
if err != nil {
t.Fatal(err)
}

n := sim.Net.GetNode(id).Node.(*adapters.SimNode)
if n.Service("noop1") == nil {
t.Error("service noop1 not found on node")
}
if n.Service("noop2") == nil {
t.Error("service noop2 not found on node")
}
}

func TestAddNodeDuplicateServiceError(t *testing.T) {
sim := New(map[string]ServiceFunc{
"noop1": noopServiceFunc,
"noop2": noopServiceFunc,
})
defer sim.Close()

wantErr := "duplicate service: *simulation.noopService"
_, err := sim.AddNode()
if err.Error() != wantErr {
t.Errorf("got error %q, want %q", err, wantErr)
}
}

func TestAddNodes(t *testing.T) {
sim := New(noopServiceFuncMap)
defer sim.Close()
Expand Down
7 changes: 7 additions & 0 deletions swarm/network/simulation/simulation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ type ServiceFunc func(ctx *adapters.ServiceContext, bucket *sync.Map) (s node.Se

// New creates a new Simulation instance with new
// simulations.Network initialized with provided services.
// Services map must have unique keys as service names and
// every ServiceFunc must return a node.Service of the unique type.
// This restriction is required by node.Node.Start() function
// which is used to start node.Service returned by ServiceFunc.
func New(services map[string]ServiceFunc) (s *Simulation) {
s = &Simulation{
buckets: make(map[enode.ID]*sync.Map),
Expand All @@ -76,6 +80,9 @@ func New(services map[string]ServiceFunc) (s *Simulation) {

adapterServices := make(map[string]adapters.ServiceFunc, len(services))
for name, serviceFunc := range services {
// Scope this variables correctly
// as they will be in the adapterServices[name] function accessed later.
name, serviceFunc := name, serviceFunc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. How did that ever work with multiple services...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that we never started all services up until now, and newly created TestAddNodeMultipleServices was a missing test case from start.

Copy link
Contributor

@nolash nolash Nov 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about the others, but I find this notation confusing. I would prefer different names in the inner scope. I believe I even deleted something similar to this somewhere else in the code a while ago, because I didn't realize what it did. (I wish now I remember what and where that was)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally disagree. Are you saying that we should change a common approach in go because you are not familiar with it? There is even a comment above. I am also interested what others think.

With different variable names, the original ones are still available and can be used instead the new ones which opens the possibility for the same issue that should be solved with scoping. With the same variable names there is no such risk.

Copy link
Contributor

@nolash nolash Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that we should change a common approach in go because you are not familiar with it?

No, that's not what I'm saying. You are right: I didn't even know it was a common approach in go. I'm sure I should have.

What I am saying is I actually deleted code like this before because it looked nonsensical, and I'm pretty sure it even slipped through code review, which it shouldn't have if it was glaringly obvious. So let's make sure everyone in the team recognizes this trick. Maybe the comment is enough, I dunno.

Apologies for my ignorance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nloash. If you have any suggestions how the comment can be improved, please provide it.

Copy link
Contributor

@nolash nolash Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've posted in hq to raise awareness.

the original ones are still available and can be used instead the new ones which opens the possibility for the same issue that should be solved with scoping.

I understand the convenience, but I really don't like the style. I believe we should explicitly know the vars we use. A comment would serve the same purpose for the contrary argument.

But then, as you say, who am I to contend with the perfect coder conditioning oracle that is Google :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think comment is fine for this notation. We should be aware of this construct and just raise awareness of it. I personally would not add a comment if I write code that needs to scope the loop vars.

https://github.com/golang/go/wiki/CommonMistakes

s.serviceNames = append(s.serviceNames, name)
adapterServices[name] = func(ctx *adapters.ServiceContext) (node.Service, error) {
b := new(sync.Map)
Expand Down
13 changes: 13 additions & 0 deletions swarm/network/simulation/simulation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,3 +205,16 @@ func (t *noopService) Start(server *p2p.Server) error {
func (t *noopService) Stop() error {
return nil
}

// a helper function for most basic noop service
// of a different type then noopService to test
// multiple services on one node.
func noopService2Func(ctx *adapters.ServiceContext, b *sync.Map) (node.Service, func(), error) {
return new(noopService2), nil, nil
}

// noopService2 is the service that does not do anything
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just embed noopService? no need to define functions then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, agree, nicer.

// but implements node.Service interface.
type noopService2 struct {
noopService
}
2 changes: 1 addition & 1 deletion swarm/network/stream/delivery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ func (d *Delivery) RequestFromPeers(ctx context.Context, req *network.Request) (
}
sp = d.getPeer(id)
if sp == nil {
log.Warn("Delivery.RequestFromPeers: peer not found", "id", id)
//log.Warn("Delivery.RequestFromPeers: peer not found", "id", id)
return true
}
spID = &id
Expand Down