Skip to content

Commit a4e930c

Browse files
authored
refactor: simplify shutdown state (#3769)
## Description Based on #3763 Tries to simplify shutdown state to be simpler to reason about. ## Breaking Changes <!-- Optional, if there are any breaking changes document them, including how to migrate older code. --> ## Notes & open questions <!-- Any notes, remarks or open questions you have to make about the PR. --> ## Change checklist <!-- Remove any that are not relevant. --> - [ ] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented. - [ ] List all breaking changes in the above "Breaking Changes" section. - [ ] Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are: - [ ] [`quic-rpc`](https://github.com/n0-computer/quic-rpc) - [ ] [`iroh-gossip`](https://github.com/n0-computer/iroh-gossip) - [ ] [`iroh-blobs`](https://github.com/n0-computer/iroh-blobs) - [ ] [`dumbpipe`](https://github.com/n0-computer/dumbpipe) - [ ] [`sendme`](https://github.com/n0-computer/sendme)
1 parent 86bf30c commit a4e930c

File tree

1 file changed

+40
-34
lines changed

1 file changed

+40
-34
lines changed

iroh/src/magicsock.rs

Lines changed: 40 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -153,14 +153,37 @@ pub(crate) struct Handle {
153153
msock: Arc<MagicSock>,
154154
// empty when shutdown
155155
actor_task: Arc<Mutex<Option<AbortOnDropHandle<()>>>>,
156-
/// Token to cancel the actor task and shutdown the relay transport.
157-
shutdown_token: CancellationToken,
158-
/// Token to cancel running netreports.
159-
shutdown_token_netreport: CancellationToken,
160156
// quinn endpoint
161157
endpoint: quinn::Endpoint,
162158
}
163159

160+
#[derive(Debug)]
161+
struct ShutdownState {
162+
at_close_start: CancellationToken,
163+
at_endpoint_closed: CancellationToken,
164+
closed: AtomicBool,
165+
}
166+
167+
impl Default for ShutdownState {
168+
fn default() -> Self {
169+
Self {
170+
at_close_start: CancellationToken::new(),
171+
at_endpoint_closed: CancellationToken::new(),
172+
closed: AtomicBool::new(false),
173+
}
174+
}
175+
}
176+
177+
impl ShutdownState {
178+
fn is_closing(&self) -> bool {
179+
self.at_close_start.is_cancelled()
180+
}
181+
182+
fn is_closed(&self) -> bool {
183+
self.closed.load(Ordering::Relaxed)
184+
}
185+
}
186+
164187
/// Iroh connectivity layer.
165188
///
166189
/// This is responsible for routing packets to endpoints based on endpoint IDs, it will initially
@@ -178,11 +201,8 @@ pub(crate) struct MagicSock {
178201
/// EndpointId of this endpoint.
179202
public_key: PublicKey,
180203

181-
// - State Management
182-
/// Close is in progress (or done)
183-
closing: AtomicBool,
184-
/// Close was called.
185-
closed: AtomicBool,
204+
// - Shutdown Management
205+
shutdown: ShutdownState,
186206

187207
// - Networking Info
188208
/// Our discovered direct addresses.
@@ -234,12 +254,12 @@ impl MagicSock {
234254
})
235255
}
236256

237-
fn is_closing(&self) -> bool {
238-
self.closing.load(Ordering::SeqCst)
257+
pub(crate) fn is_closed(&self) -> bool {
258+
self.shutdown.is_closed()
239259
}
240260

241-
pub(crate) fn is_closed(&self) -> bool {
242-
self.closed.load(Ordering::SeqCst)
261+
fn is_closing(&self) -> bool {
262+
self.shutdown.is_closing()
243263
}
244264

245265
/// Get the cached version of addresses.
@@ -682,7 +702,7 @@ impl DirectAddrUpdateState {
682702
debug!("starting direct addr update ({:?})", why);
683703
// Don't start a net report probe if we know
684704
// we are shutting down
685-
if self.msock.is_closing() || self.msock.is_closed() || self.shutdown_token.is_cancelled() {
705+
if self.shutdown_token.is_cancelled() {
686706
debug!("skipping net_report, socket is shutting down");
687707
// deactivate portmapper
688708
self.port_mapper.deactivate();
@@ -861,9 +881,8 @@ impl Handle {
861881

862882
let msock = Arc::new(MagicSock {
863883
public_key: secret_key.public(),
864-
closing: AtomicBool::new(false),
865-
closed: AtomicBool::new(false),
866884
actor_sender: actor_sender.clone(),
885+
shutdown: ShutdownState::default(),
867886
ipv6_reported,
868887
remote_map,
869888
discovery,
@@ -939,15 +958,14 @@ impl Handle {
939958
);
940959

941960
let (direct_addr_done_tx, direct_addr_done_rx) = mpsc::channel(8);
942-
let shutdown_token_netreport = CancellationToken::new();
943961
let direct_addr_update_state = DirectAddrUpdateState::new(
944962
msock.clone(),
945963
#[cfg(not(wasm_browser))]
946964
port_mapper,
947965
Arc::new(AsyncMutex::new(net_reporter)),
948966
relay_map,
949967
direct_addr_done_tx,
950-
shutdown_token_netreport.clone(),
968+
msock.shutdown.at_close_start.child_token(),
951969
);
952970

953971
let netmon_watcher = network_monitor.interface_state();
@@ -979,8 +997,6 @@ impl Handle {
979997
msock,
980998
actor_task,
981999
endpoint,
982-
shutdown_token,
983-
shutdown_token_netreport,
9841000
})
9851001
}
9861002

@@ -1003,18 +1019,8 @@ impl Handle {
10031019
return;
10041020
}
10051021

1006-
// Only set closing if not already set
1007-
if self
1008-
.msock
1009-
.closing
1010-
.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed)
1011-
.is_err()
1012-
{
1013-
return;
1014-
}
1015-
1016-
// Cancel any running netreports
1017-
self.shutdown_token_netreport.cancel();
1022+
// Cancel at_close_start token, which cancels running netreports.
1023+
self.msock.shutdown.at_close_start.cancel();
10181024

10191025
// Initiate closing all connections, and refuse future connections.
10201026
self.endpoint.close(0u16.into(), b"");
@@ -1039,7 +1045,7 @@ impl Handle {
10391045
trace!("wait_idle done");
10401046

10411047
// Start cancellation of all actors
1042-
self.shutdown_token.cancel();
1048+
self.msock.shutdown.at_endpoint_closed.cancel();
10431049

10441050
// MutexGuard is not held across await points
10451051
let task = self.actor_task.lock().expect("poisoned").take();
@@ -1060,7 +1066,7 @@ impl Handle {
10601066
}
10611067
}
10621068

1063-
self.msock.closed.store(true, Ordering::SeqCst);
1069+
self.msock.shutdown.closed.store(true, Ordering::SeqCst);
10641070

10651071
trace!("magicsock closed");
10661072
}

0 commit comments

Comments
 (0)