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

lntest: avoid global ServeMux #5674

Merged
merged 1 commit into from Oct 13, 2021

Conversation

torkelrogstad
Copy link
Contributor

@torkelrogstad torkelrogstad commented Aug 27, 2021

Using the default, global ServeMux prevents the same process from
calling lntest.NewNetworkHarness multiple times, because we get a
panic when registering HTTP routes.

Instead, we use the ServeMux belonging to the fee service struct.

Pull Request Checklist

  • All changes are Go version 1.15 compliant
  • Your PR passes all CI checks. If a check cannot be passed for a justifiable reason, that reason must be stated in the commit message and PR description.
  • If this is your first time contributing, we recommend you read the Code Contribution Guidelines
  • For bug fixes: If possible, code is accompanied by new tests which trigger the bug being fixed to prevent regressions
    I could add a new test that calls lntest.NewNetworkHarness twice, but that sounds like a slow/expensive test for little benefit...
  • Any new logging statements use an appropriate subsystem and logging level
  • For code and documentation: lines are wrapped at 80 characters (the tab character should be counted as 8 characters, not 4, as some IDEs do per default)
  • A description of your changes should be added to running the release notes for the milestone your change will land in.

Using the default, global ServeMux prevents the same process from
calling `lntest.NewNetworkHarness` multiple times, because we get a
panic when registering HTTP routes.

Instead, we use the ServeMux beloning to the fee service struct.
@philipglazman
Copy link

Ran into this issue as well. Confirming your PR fixes the root issue.

@torkelrogstad
Copy link
Contributor Author

Possible to get this in @Crypt-iQ @Roasbeef, after I update the release notes?

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, LGTM 🎉

Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

LGTM⚡️

@guggero guggero merged commit 5d7e814 into lightningnetwork:master Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants