app-server: prepare to run initialized rpcs concurrently#17372
app-server: prepare to run initialized rpcs concurrently#17372euroelessar merged 2 commits intomainfrom
Conversation
euroelessar
force-pushed
the
codex/concurrent-rpcs-initialized-rpcs
branch
from
2435f00 to
8514a9b
Compare
euroelessar
changed the title
euroelessar
changed the title
euroelessar
force-pushed
the
codex/concurrent-rpcs-initialized-rpcs
branch
from
8514a9b to
5c883c1
Compare
| Arc::clone(&connection_state.session), | ||
| ) | ||
| .await; | ||
| let opted_out_notification_methods_snapshot = connection_state |
There was a problem hiding this comment.
Is it still necessary now that the session state is immutable after init?
There was a problem hiding this comment.
it's a bit bigger change to change this (e.g. I would need to remove ConnectionState, plumb the Arc<ConnectionSessionState> throughout the stack, etc)
ok if I do it in a followup?
There was a problem hiding this comment.
Yes but please really do it as a follow-up :D
| return; | ||
| } | ||
|
|
||
| let app_server_client_name = session.app_server_client_name().map(str::to_string); |
There was a problem hiding this comment.
ultra nit: I would defer this clone till the code actually detaches work
|
|
||
| pub(crate) async fn process_request( | ||
| &self, | ||
| self: &Arc<Self>, |
There was a problem hiding this comment.
This Arc receiver leaks the future concurrency implementation detail into every caller, this is strange. I would keep an &self and Arc it only at the spawn site
There was a problem hiding this comment.
I can't promote &self into Arc<Self> without storing weak pointer on itself or similar as a field, but that would be even more strange?
alternatively we can move all the fields into a new MessageProcessorState or similar, but that does't provide clean benefits
There was a problem hiding this comment.
Ideally we have MessageProcessor as cheap cloneable handle we hide an Arced inner in it
But can be done in the same follow-up
There was a problem hiding this comment.
sounds good, will add inner in the followup then
euroelessar
force-pushed
the
codex/concurrent-rpcs-initialized-rpcs
branch
from
551d1b7 to
7e5a28b
Compare
| Arc::clone(&connection_state.session), | ||
| ) | ||
| .await; | ||
| let opted_out_notification_methods_snapshot = connection_state |
There was a problem hiding this comment.
Yes but please really do it as a follow-up :D
|
|
||
| pub(crate) async fn process_request( | ||
| &self, | ||
| self: &Arc<Self>, |
There was a problem hiding this comment.
Ideally we have MessageProcessor as cheap cloneable handle we hide an Arced inner in it
But can be done in the same follow-up
2 participants
Summary
MessageProcessorand per-connection session state so initialized service RPC handling can be moved into spawned tasks in a follow-up PR.Arc/OnceLockinstead of mutable borrowed connection state.tokio::spawnfor service RPCs yet.Testing
just fmtcargo test -p codex-app-server(fails on existing hardening gaps covered by app-server: tolerate shell startup output in shell command tests #17375, app-server: harden realtime tests against nondeterminism #17376, and app-server: assert disabled analytics event precisely #17377; the pipelined config regression passed before the unrelated failures)just fix -p codex-app-server