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.

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

Unspecified initial firing of event.EvtLocalAddressesUpdated #991

Closed
aschmahmann opened this issue Aug 17, 2020 · 3 comments · Fixed by #2016
Closed

Unspecified initial firing of event.EvtLocalAddressesUpdated #991

aschmahmann opened this issue Aug 17, 2020 · 3 comments · Fixed by #2016
Assignees
Labels
kind/bug A bug in existing code (including security flaws)

Comments

@aschmahmann
Copy link
Collaborator

Perhaps this issue belong in go-libp2p-core, but it's not obvious to me whether event.EvtLocalAddressesUpdated should be fired when initially creating a Host.

For example, the BasicHost will fire this event in transitioning from nil addresses to found addresses during its first run of the background function

emitAddrChange(curr, lastAddrs)

However, because this is a background event this may occur before or after any systems that use a host have been created. For uses such as testing which likely depend on knowing exactly how and when events are fired this can be a bit of a mine field.

For example, #936 and #956 introduced the use of the netroute library in determining host addresses. As netroute results in some OS calls that can take time this made tests that assumed event.EvtLocalAddressesUpdated was not fired between creating the Host and creating the subsystem that depends on the Host fail.

It seems to me like either we should make event.EvtLocalAddressesUpdated not be fired when the BasicHost first gets an address or we should make this event Stateful. Leaving the event in this middle ground makes testing difficult due to asynchronicity issues.

@petar @willscott @raulk

@aschmahmann aschmahmann added the kind/bug A bug in existing code (including security flaws) label Aug 17, 2020
@lthibault
Copy link
Contributor

lthibault commented Sep 23, 2020

It seems to me like either we should make event.EvtLocalAddressesUpdated not be fired when the BasicHost first gets an address or we should make this event Stateful.

I currently rely on this behavior, so I would strongly prefer the Stateful approach.

However, because this is a background event this may occur before or after any systems that use a host have been created.

I can also confirm that this is a bit of a hassle. In my own usage, I've resorted to first starting the Host without any listen addresses, and then manually calling l.Network().Listen(addrs...) to generate EvtLocalAddressesUpdated. Having a stateful event subscription would allow me to tear out quite a bit of convoluted code.

@willscott
Copy link
Contributor

making it Stateful seems like the right way to go in terms of how i'd think of it. Makes the naming of the event a bit nonintuitive, but still probably the best compromise

@aarshkshah1992 aarshkshah1992 self-assigned this Oct 28, 2020
@marten-seemann
Copy link
Contributor

I checked that the initial listen addresses are emitted as an event, and this is reliable, since with #1147 the event is stateful. I'd like to add an integration test for this before closing this issue though, since this is a pretty important behavior that users rely on. PR incoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants