-
Notifications
You must be signed in to change notification settings - Fork 6.2k
[app-server] Add cancellation registry and $/cancelRequest #6064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| use std::collections::HashMap; | ||
| use std::sync::Arc; | ||
| use std::sync::Mutex as StdMutex; | ||
| use std::sync::PoisonError; | ||
|
|
||
| use codex_app_server_protocol::RequestId; | ||
|
|
||
| trait Cancellable: Send { | ||
| fn cancel(&self); | ||
| } | ||
|
|
||
| impl<F> Cancellable for F | ||
| where | ||
| F: Fn() + Send, | ||
| { | ||
| fn cancel(&self) { | ||
| (self)(); | ||
| } | ||
| } | ||
|
|
||
| #[derive(Clone, Default)] | ||
| pub(crate) struct CancellationRegistry { | ||
| inner: Arc<StdMutex<HashMap<RequestId, Box<dyn Cancellable + Send>>>>, | ||
| } | ||
|
|
||
| impl CancellationRegistry { | ||
| pub(crate) fn insert<F>(&self, id: RequestId, f: F) | ||
| where | ||
| F: Fn() + Send + 'static, | ||
| { | ||
| self.inner | ||
| .lock() | ||
| .unwrap_or_else(PoisonError::into_inner) | ||
| .insert(id, Box::new(f)); | ||
| } | ||
|
Comment on lines
+27
to
+35
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new registry only accepts Useful? React with 👍 / 👎. |
||
|
|
||
| pub(crate) fn cancel(&self, id: &RequestId) -> bool { | ||
| // Remove the callback while holding the lock, but invoke it only after | ||
| // releasing the lock to avoid deadlocks or long critical sections. | ||
| let callback = { | ||
| let mut guard = self.inner.lock().unwrap_or_else(PoisonError::into_inner); | ||
| guard.remove(id) | ||
| }; | ||
| if let Some(c) = callback { | ||
| c.cancel(); | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn remove(&self, id: &RequestId) { | ||
| let mut guard = self.inner.lock().unwrap_or_else(PoisonError::into_inner); | ||
| guard.remove(id); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah very interesting that this is done as a notification. makes sense after reading https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#cancelRequest
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, looks like this might not be applicable to login. the use case for a
$/cancelRequestcall seems to be:so this seems generally useful to interrupt long-running operations.
whereas with chatgpt login, we do:
the auth URL looks like:
https://auth.openai.com/oauth/authorize?response_type=code&client_id=app_EMoamEEZ73f0CkXaXp7hrann&redirect_uri=http%3A%2F%2Flocalhost%3A1455%2Fauth%2Fcallback&scope=openid%20profile%20email%20offline_access&code_challenge=[code]&code_challenge_method=S256&id_token_add_organizations=true&codex_cli_simplified_flow=true&state=[state]&originator=codex_cli_rs
So the codex process starts up an HTTP server on port 1455 to listen for the redirect after login is successful, and then update its auth storage. Asking codex, looks like it'll time out in 10 mins if user doesn't complete login flow and there's no cancellation. We expose the
CancelChatGptLoginrequest as a way to immedately cancel the login, mostly for UX I think (i.e. they can render a "cancel login" button in their UI).tldr my 2c: based on our convo with pavel, I think we actually want to name it
account/login/startand pair it withaccount/login/cancelsince the login request returns immediately with a URLThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the back and forth on this!