fix: reduce references to Server in codex-login crate (#2398)
Updates the tokio task that monitors `shutdown_notify` and server requests to ensure that `server.unblock()` is always called, which means that `ShutdownHandle` only has to invoke `notify_waiters()`. Now `LoginServer` no longer has to maintain a reference to `Server`. The `Arc<Server>` only has two active references: the `thread::spawn()` for reading server messages and the `tokio::task()` that consumes them (and the shutdown message). Now when shutdown is called (or if login completes successfully), the `server.unblock()` call ensures the thread terminates cleanly, which in turn ensures `rx.recv()` in the `tokio::spawn()` returns `Err`, causing the `tokio::task()` to exit cleanly, as well. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/2398). * #2399 * __->__ #2398 * #2396 * #2395 * #2394 * #2393 * #2389
This commit is contained in:
@@ -66,24 +66,14 @@ impl LoginServer {
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
#[derive(Clone, Debug)]
|
||||
pub struct ShutdownHandle {
|
||||
shutdown_notify: Arc<tokio::sync::Notify>,
|
||||
server: Arc<Server>,
|
||||
}
|
||||
|
||||
impl std::fmt::Debug for ShutdownHandle {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
f.debug_struct("ShutdownHandle")
|
||||
.field("shutdown_notify", &self.shutdown_notify)
|
||||
.finish()
|
||||
}
|
||||
}
|
||||
|
||||
impl ShutdownHandle {
|
||||
pub fn shutdown(&self) {
|
||||
self.shutdown_notify.notify_waiters();
|
||||
self.server.unblock();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -133,14 +123,14 @@ pub fn run_login_server(
|
||||
let shutdown_notify = shutdown_notify.clone();
|
||||
let server = server.clone();
|
||||
tokio::spawn(async move {
|
||||
loop {
|
||||
let result = loop {
|
||||
tokio::select! {
|
||||
_ = shutdown_notify.notified() => {
|
||||
return Err(io::Error::other("Login was not completed"));
|
||||
break Err(io::Error::other("Login was not completed"));
|
||||
}
|
||||
maybe_req = rx.recv() => {
|
||||
let Some(req) = maybe_req else {
|
||||
return Err(io::Error::other("Login was not completed"));
|
||||
break Err(io::Error::other("Login was not completed"));
|
||||
};
|
||||
|
||||
let url_raw = req.url().to_string();
|
||||
@@ -159,13 +149,16 @@ pub fn run_login_server(
|
||||
}
|
||||
|
||||
if is_login_complete {
|
||||
shutdown_notify.notify_waiters();
|
||||
server.unblock();
|
||||
return Ok(());
|
||||
break Ok(());
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
// Ensure that the server is unblocked so the thread dedicated to
|
||||
// running `server.recv()` in a loop exits cleanly.
|
||||
server.unblock();
|
||||
result
|
||||
})
|
||||
};
|
||||
|
||||
@@ -173,10 +166,7 @@ pub fn run_login_server(
|
||||
auth_url,
|
||||
actual_port,
|
||||
server_handle,
|
||||
shutdown_handle: ShutdownHandle {
|
||||
shutdown_notify,
|
||||
server,
|
||||
},
|
||||
shutdown_handle: ShutdownHandle { shutdown_notify },
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user