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

Use then instead of then_some when checking upnp_enabled to avoid useless UPnP query #6170

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

dknopik
Copy link
Contributor

@dknopik dknopik commented Jul 24, 2024

Issue Addressed

I noticed that Lighthouse sends to UPnP's multicast address 239.255.255.250 even though I passed --disable-upnp. After some investigation, I found out that libp2p_upnp's Behaviour::default immediately starts the search for the gateway. As we use bool::then_some to transform upnp_enabled into an Option<Behaviour>, we have to call Behaviour::default eagerly to pass it, triggering the search even if upnp_enabled is false.

Proposed Changes

Use bool::then instead, passing Behaviour::default as function reference so that it is called lazily (only when upnp_enabled is true).

Additional Info

In my opinion, starting the search immediately when Behaviour::default is invoked is bad design (as usually default only returns "some data"), so an alternative fix is to try and improve the API upstream.

@jimmygchen jimmygchen added ready-for-review The code is ready for review Networking labels Jul 24, 2024
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Nice find. Thanks for the PR <3

@AgeManning
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Jul 25, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at a2ab26c

mergify bot added a commit that referenced this pull request Jul 25, 2024
@mergify mergify bot merged commit a2ab26c into sigp:unstable Jul 25, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Networking ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants