[Merged by Bors] - Prevent port re-use in HTTP API tests by michaelsproul · Pull Request #4745 · sigp/lighthouse

michaelsproul · 2023-09-18T07:32:23Z

Issue Addressed

CI is plagued by AddrAlreadyInUse failures, which are caused by race conditions in allocating free ports.

This PR removes all usages of the unused_port crate for Lighthouse's HTTP API, in favour of passing :0 as the listen address. As a result, the listen address isn't known ahead of time and must be read from the listening socket after it binds. This requires tying some self-referential knots, which is a little disruptive, but hopefully doesn't clash too much with Deneb 🤞

There are still a few usages of unused_tcp4_port left in cases where we start external processes, like the watch Postgres DB, Anvil, Geth, Nethermind, etc. Removing these usages is non-trivial because it's hard to read the port back from an external process after starting it with --port 0. We might be able to do something on Linux where we read from /proc/, but I'll leave that for future work.

michaelsproul · 2023-09-19T07:03:09Z

There's a new ArcSwap introduced in the execution layer which needs to exist to break the self-referential cycle in testing. I think it shouldn't have a noticeable performance penalty for real nodes, but could be something to keep an eye on.

We're using ArcSwap::load_full at the moment, but could use a lighter-weight method with a bit of type plumbing in future if we want moar speed.

michaelsproul · 2023-09-19T08:29:10Z

Looks like unused port needs deleting from the Cargo.toml 😎 I'll do that shortly

michaelsproul · 2023-09-19T13:27:07Z

Ready for review, and CI is green 🚀

AgeManning

CI is green, this mainly touches tests, so LGTM.

Keen to get this in.

Also.... YESSSS!!!

jimmygchen

Looks good! Thanks 👍

jimmygchen · 2023-09-20T00:28:40Z

Lets' gooo 🚀

michaelsproul · 2023-09-20T00:31:44Z

🤞

bors r+

## Issue Addressed CI is plagued by `AddrAlreadyInUse` failures, which are caused by race conditions in allocating free ports. This PR removes all usages of the `unused_port` crate for Lighthouse's HTTP API, in favour of passing `:0` as the listen address. As a result, the listen address isn't known ahead of time and must be read from the listening socket after it binds. This requires tying some self-referential knots, which is a little disruptive, but hopefully doesn't clash too much with Deneb 🤞 There are still a few usages of `unused_tcp4_port` left in cases where we start external processes, like the `watch` Postgres DB, Anvil, Geth, Nethermind, etc. Removing these usages is non-trivial because it's hard to read the port back from an external process after starting it with `--port 0`. We might be able to do something on Linux where we read from `/proc/`, but I'll leave that for future work.

bors · 2023-09-20T01:15:21Z

Build failed:

release-tests-windows

AgeManning · 2023-09-20T01:18:46Z

bors retry

## Issue Addressed CI is plagued by `AddrAlreadyInUse` failures, which are caused by race conditions in allocating free ports. This PR removes all usages of the `unused_port` crate for Lighthouse's HTTP API, in favour of passing `:0` as the listen address. As a result, the listen address isn't known ahead of time and must be read from the listening socket after it binds. This requires tying some self-referential knots, which is a little disruptive, but hopefully doesn't clash too much with Deneb 🤞 There are still a few usages of `unused_tcp4_port` left in cases where we start external processes, like the `watch` Postgres DB, Anvil, Geth, Nethermind, etc. Removing these usages is non-trivial because it's hard to read the port back from an external process after starting it with `--port 0`. We might be able to do something on Linux where we read from `/proc/`, but I'll leave that for future work.

michaelsproul · 2023-09-20T01:26:32Z

That port re-use error should be fixed by #4700. If this PR fails again, can we merge both together in the same batch?

bors · 2023-09-20T02:12:05Z

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

## Issue Addressed #4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - #4705 - #4402 - #4745

## Issue Addressed CI is plagued by `AddrAlreadyInUse` failures, which are caused by race conditions in allocating free ports. This PR removes all usages of the `unused_port` crate for Lighthouse's HTTP API, in favour of passing `:0` as the listen address. As a result, the listen address isn't known ahead of time and must be read from the listening socket after it binds. This requires tying some self-referential knots, which is a little disruptive, but hopefully doesn't clash too much with Deneb 🤞 There are still a few usages of `unused_tcp4_port` left in cases where we start external processes, like the `watch` Postgres DB, Anvil, Geth, Nethermind, etc. Removing these usages is non-trivial because it's hard to read the port back from an external process after starting it with `--port 0`. We might be able to do something on Linux where we read from `/proc/`, but I'll leave that for future work.

## Issue Addressed sigp#4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - sigp#4705 - sigp#4402 - sigp#4745

CI is plagued by `AddrAlreadyInUse` failures, which are caused by race conditions in allocating free ports. This PR removes all usages of the `unused_port` crate for Lighthouse's HTTP API, in favour of passing `:0` as the listen address. As a result, the listen address isn't known ahead of time and must be read from the listening socket after it binds. This requires tying some self-referential knots, which is a little disruptive, but hopefully doesn't clash too much with Deneb 🤞 There are still a few usages of `unused_tcp4_port` left in cases where we start external processes, like the `watch` Postgres DB, Anvil, Geth, Nethermind, etc. Removing these usages is non-trivial because it's hard to read the port back from an external process after starting it with `--port 0`. We might be able to do something on Linux where we read from `/proc/`, but I'll leave that for future work.

## Issue Addressed sigp#4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - sigp#4705 - sigp#4402 - sigp#4745

CI is plagued by `AddrAlreadyInUse` failures, which are caused by race conditions in allocating free ports. This PR removes all usages of the `unused_port` crate for Lighthouse's HTTP API, in favour of passing `:0` as the listen address. As a result, the listen address isn't known ahead of time and must be read from the listening socket after it binds. This requires tying some self-referential knots, which is a little disruptive, but hopefully doesn't clash too much with Deneb 🤞 There are still a few usages of `unused_tcp4_port` left in cases where we start external processes, like the `watch` Postgres DB, Anvil, Geth, Nethermind, etc. Removing these usages is non-trivial because it's hard to read the port back from an external process after starting it with `--port 0`. We might be able to do something on Linux where we read from `/proc/`, but I'll leave that for future work.

## Issue Addressed sigp#4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - sigp#4705 - sigp#4402 - sigp#4745

Use 0 ports in mock builder

c311a61

michaelsproul added work-in-progress PR is a work-in-progress HTTP-API infra-ci v4.5.0 ETA Q4 2023 labels Sep 18, 2023

A nice easy one

5723d0f

jmcph4 mentioned this pull request Sep 19, 2023

[Merged by Bors] - Allow libp2p to determine listening addresses #4700

Closed

michaelsproul added 2 commits September 19, 2023 16:18

Break dependence between BN<>EL<>builder

1bae6b5

Simplify

b4b0774

michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Sep 19, 2023

Move builder back into Inner

2568d18

Fix cargo-udeps

34fe5fe

AgeManning approved these changes Sep 20, 2023

View reviewed changes

AgeManning added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 20, 2023

jimmygchen self-requested a review September 20, 2023 00:23

jimmygchen approved these changes Sep 20, 2023

View reviewed changes

bors bot changed the title ~~Prevent port re-use in HTTP API tests~~ [Merged by Bors] - Prevent port re-use in HTTP API tests Sep 20, 2023

bors bot closed this Sep 20, 2023

michaelsproul deleted the http-api-ports branch September 20, 2023 02:13

jmcph4 mentioned this pull request Dec 20, 2023

Fix zero port error #5021

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Prevent port re-use in HTTP API tests#4745

[Merged by Bors] - Prevent port re-use in HTTP API tests#4745
michaelsproul wants to merge 6 commits intosigp:unstablefrom
michaelsproul:http-api-ports

michaelsproul commented Sep 18, 2023 •

edited

Loading

Uh oh!

michaelsproul commented Sep 19, 2023

Uh oh!

michaelsproul commented Sep 19, 2023

Uh oh!

michaelsproul commented Sep 19, 2023

Uh oh!

AgeManning left a comment

Uh oh!

jimmygchen left a comment

Uh oh!

jimmygchen commented Sep 20, 2023

Uh oh!

michaelsproul commented Sep 20, 2023

Uh oh!

bors bot commented Sep 20, 2023

Uh oh!

AgeManning commented Sep 20, 2023

Uh oh!

michaelsproul commented Sep 20, 2023

Uh oh!

bors bot commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

michaelsproul commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Uh oh!

michaelsproul commented Sep 19, 2023

Uh oh!

michaelsproul commented Sep 19, 2023

Uh oh!

michaelsproul commented Sep 19, 2023

Uh oh!

AgeManning left a comment

Choose a reason for hiding this comment

Uh oh!

jimmygchen left a comment

Choose a reason for hiding this comment

Uh oh!

jimmygchen commented Sep 20, 2023

Uh oh!

michaelsproul commented Sep 20, 2023

Uh oh!

bors bot commented Sep 20, 2023

Uh oh!

AgeManning commented Sep 20, 2023

Uh oh!

michaelsproul commented Sep 20, 2023

Uh oh!

bors bot commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaelsproul commented Sep 18, 2023 •

edited

Loading