Listen on all provided addresses #644
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
| listening_logger, | ||
| "Stopping listening to inbound connections." | ||
| let logger = Arc::clone(&listening_logger); | ||
| let listeners = self.runtime.block_on(async move { |
There was a problem hiding this comment.
Need to block if we want to return an error.
There was a problem hiding this comment.
Hmm, I see the point. I guess we need to if we want to abort on error for now, but we'll probably make start async soonish anyways.
There was a problem hiding this comment.
Alternative is to use non-tokio binds here.
| listening_logger, | ||
| "Stopping listening to inbound connections." | ||
| let logger = Arc::clone(&listening_logger); | ||
| let listeners = self.runtime.block_on(async move { |
There was a problem hiding this comment.
Hmm, I see the point. I guess we need to if we want to abort on error for now, but we'll probably make start async soonish anyways.
src/lib.rs
Outdated
| res = listener.accept() => { | ||
| let tcp_stream = res.unwrap().0; | ||
| let peer_mgr = Arc::clone(&peer_mgr); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Mhh, looks pre-existing, but we should probably also use spawn_cancellable_background_task for this.
src/lib.rs
Outdated
| for addr in &*bind_addrs { | ||
| match tokio::net::TcpListener::bind(addr).await { | ||
| Ok(listener) => { | ||
| log_trace!(logger, "Bound to {}", addr); |
There was a problem hiding this comment.
Can we make this message a bit more verbose?
There was a problem hiding this comment.
Quite vague indeed. Made it Listener bound to {}
joostjager
force-pushed
the
multi-listen-fix
branch
2 times, most recently
from
167555c to
b1cc7f3
Compare
|
With both ports being bound, the chances of a failure increase. I've added a A good fix might be to pick ports from a range that doesn't include the ephemeral ports. |
joostjager
force-pushed
the
multi-listen-fix
branch
from
a47bee3 to
d8f967c
Compare
|
Removed the port range change from this PR again, needs further investigation. |
There was a problem hiding this comment.
Changes here look good, maybe could use a test to check we're listening on all ports as expected? Although maybe it's trivial/redundant?
More generally, where are we at with this? Do you still intend to investigate the port collisions etc. and include fixes as part of this PR, or do you intend to split that out?
|
Test added.
Split it out, I think we can get this out of the way first. |
joostjager
force-pushed
the
multi-listen-fix
branch
2 times, most recently
from
1bbf1bb to
54bed69
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
Now that we established that in any case we need to pick random ports from the lower half, I've also added that commit here. |
So that we'll cleanly signal and wait for termination.
joostjager
force-pushed
the
multi-listen-fix
branch
from
5d412c3 to
4c718c6
Compare
|
Rebased |
Previously ldk-node would start binding after the first successful bind to an address.
To avoid conflicts ("port in use") with ports that were used for
outgoing connections and are now in the TIME_WAIT state.
joostjager
force-pushed
the
multi-listen-fix
branch
from
4c718c6 to
0f621ff
Compare
|
Squashed |
3 participants
Previously ldk-node would start binding after the first successful bind to an address.
Also fixes #318