-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
net/mock: mimic Swarm's event and notification behavior in MockNet #2287
Conversation
// notify asynchronously to mimic Swarm | ||
// FIXME: IIRC, we wanted to make notify for Close synchronous |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and yes, this means that ideally, we'd also get rid of the Go routine for the Disconnected notification.
go func() { | ||
c.notifLk.Lock() | ||
defer c.notifLk.Unlock() | ||
c.net.notifyAll(func(n network.Notifiee) { | ||
n.Disconnected(c.net, c) | ||
}) | ||
}() | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved for nicer logic grouping.
p2p/net/mock/mock_net.go
Outdated
EventBus: bus, | ||
} | ||
|
||
h, err := bhost.NewHost(n, opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use lightweight blankhost here isntead?
Doesn't seem like basichost gives anything here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I'd prefer this to behave as similar to a normal node as possible. So I'd lean towards keeping this as the basic host. Is there something in basichost that's too heavy for you? I wonder if it would make more sense to make that lighter instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing specific, just wonderting. BasicHost also works
Thank you @Wondertan 🙏 |
Fixes #2267 + synchronously notifies as Swarm after #1562