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.
| peer_store: Arc<PeerStore<Arc<Logger>>>, | ||
| payment_store: Arc<PaymentStore>, | ||
| is_running: Arc<RwLock<bool>>, | ||
| is_listening: Arc<AtomicBool>, |
There was a problem hiding this comment.
Huh, please don't drop the is_listening field, we also support nodes that don't allow any inbound connections and want to indicate whether we're listening or not to a port or not here.
There was a problem hiding this comment.
Discussed offline. This would just be a reflection of the config set.
There was a problem hiding this comment.
Kept it in, but dropped the polling for it which is no longer necessary.
| 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 {}
So that we'll cleanly signal and wait for termination.
Previously ldk-node would start binding after the first successful bind to an address.
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
from
749a527 to
1bbf1bb
Compare
joostjager
force-pushed
the
multi-listen-fix
branch
from
1bbf1bb to
54bed69
Compare
Previously ldk-node would start binding after the first successful bind to an address.
Also fixes #318