Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Make dashboard bind to IPv4 & IPv6 when available #5699

Closed
wants to merge 4 commits into from

Conversation

benjamincburns
Copy link
Contributor

@benjamincburns benjamincburns commented Nov 14, 2022

PR description

Addresses issue #5690.

This change makes it so that the dashboard server and dashboard message bus listens on both IPv4 and IPv6 for a given hostname. In the event that one of the two protocols is not available for a given hostname, it will only listen on the protocol that is available.

Testing instructions

  1. Run truffle dashboard in your project of choice.
  2. Ensure that you can connect to it via http://127.0.0.1:24012 (note the port number may be different on your machine)
  3. Ensure that you can connect to it via http://[::1]:24012 (note the port number may be different on your machine)
  4. Configure the dashboard network to connect to host 127.0.0.1 and ensure that truffle migrate --reset --network dashboard succeeds under this configuration.
  5. Configure the dashboard network to connect to host ::1 and ensure that truffle migrate --reset --network dashboard succeeds under this configuration.
  6. Configure the dashboard network to connect to host localhost and ensure that truffle migrate --reset --network dashboard succeeds under this configuration.

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

Breaking changes and new features

  • I have added any needed breaking-change and new-feature labels for the appropriate packages.

Note to reviewers

There's a bit of unfortunate code duplication here. I would have preferred to put the logic for resolving the IPs of a given local hostname into the @truffle/dashboard-message-bus-common package, but the webpack operation in the build for the dashboard frontend choked on the node-specific modules that are used by this logic (the os and net modules). As a result, I decided it was easier to temporarily duplicate this code between the utils module in the dashboard server, and the utils module in the dashboard-message-bus module.

That said, we're still not breaking "the rule of three" here, so hopefully this can be approved as-is. However if #5621 happens to be ready to merge before this PR, I can rebase this code on develop, move the logic to @dashboard-message-bus-common, and drop the net and os packages in the dashboard's newly-ejected webpack config.

@OnlyOneJMJQ
Copy link

Will give this a try today and get back to you!

@benjamincburns benjamincburns marked this pull request as ready for review November 15, 2022 00:10
@benjamincburns
Copy link
Contributor Author

I think it's safe to review this while we're waiting on feedback from Josh, as it's a fairly small change and I've already verified by other means (lsof -i -P | grep LISTEN) that it has the intended result (listening on both IPv6 and IPv4 when available).

@benjamincburns
Copy link
Contributor Author

Just confirming that @OnlyOneJMJQ got back to me via Slack and confirmed that this change does indeed fix the issue he observed in #5690

packages/dashboard/lib/DashboardServer.ts Outdated Show resolved Hide resolved
packages/dashboard-message-bus/lib/utils.ts Outdated Show resolved Hide resolved
@benjamincburns
Copy link
Contributor Author

Just realized that I need to add some doc comments and improve a bit of the error handling so we're not throwing cryptic ENOTFOUND errors at users - will push a quick fix for that.

@benjamincburns
Copy link
Contributor Author

Argh, so this is frustrating... I added better error handling for the case when the user mistypes their desired hostname, but it turns out that the error handling code is unreachable due to how we provision additional ports for the websocket servers. That is, when I purposefully test with a mistyped hostname, it throws Error: getaddrinfo ENOTFOUND from that line.

The bigger issue is that the getPort function that we're using can't find an unused port that's common between multiple local IP addresses. If I call it twice (once for IPv6 and once for IPv4) it may return different ports.

I think this means that in order to make this change properly I need to refactor the message bus to reuse the http.Server instance(s) that DashboardServer sets up to serve up the dashboard webpage.

cc @cliffoo as FYI

 - Removes the need to listen on three ports to run the dashboard
 - Removes the problematic port allocation strategy for the
   publish/subscribe server implementations
@eggplantzzz
Copy link
Contributor

Closing this for now as the changes here are too big for something that we are not able to reliably reproduce on our end. If we can reproduce this in the future we'll revisit it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants