[Prototype] Add multi-connection IPC support to pyrefly TSP#3199
[Prototype] Add multi-connection IPC support to pyrefly TSP#3199heejaechang wants to merge 1 commit intofacebook:mainfrom
Conversation
Teach the server to advertise and manage extra read-only channels for Pylance, and restore the numeric protocol wire form so cross-repo type round-tripping stays compatible with the shared contract.
|
Hi @heejaechang! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
some initial thoughts, but checking it out now to look closer
| // but will only respond to TSP protocol requests | ||
| capabilities(indexing_mode, initialization_params) | ||
| let mut result = capabilities(indexing_mode, initialization_params); | ||
| result.base.experimental = Some(serde_json::json!({ |
There was a problem hiding this comment.
instead of exposing all of base, can we instead add a enable_multi_connection API and call it here?
| )) | ||
| })?; | ||
|
|
||
| let extra_server = Self::with_connection( |
There was a problem hiding this comment.
clever to use an Arc server in order for both servers to use the same memory. If we keep this approach (I wonder about instead abstracting around connections), we should document it clearly
There was a problem hiding this comment.
I am testing an alternate approach here: 9e45b56
what do you think?
| ); | ||
| drop(extra_connections); | ||
|
|
||
| std::thread::spawn(move || { |
There was a problem hiding this comment.
nit: can we abstract this out of here?
| } | ||
| } | ||
|
|
||
| fn get_pipe_name(&self, params: &ConnectionRequestParams) -> Result<String, ResponseError> { |
There was a problem hiding this comment.
this would be a lot cleaner within the same module as ConnectionRequestParams. but I guess the protocol generation script doesn't allow that?
| }) | ||
| } | ||
|
|
||
| fn handle_extra_request<'a>( |
There was a problem hiding this comment.
it's sketchy that a non-extra server can still call this function. maybe we need two server structs with two impls
| } | ||
|
|
||
| pub fn ipc(pipe_name: &str) -> std::io::Result<(Self, MessageReader, IoThread)> { | ||
| #[cfg(windows)] |
There was a problem hiding this comment.
I didn't realize this is OS-dependent. did we consider TCP? it seems like it could be simpler + just as fast in practice?
There was a problem hiding this comment.
chose IPC so we don't need to worry about exposing the connection to remote? (also things like vscode will auto detect tcp port being opened, and try to open forward channel by default and etc for remote vscode case) we could support tcp as well, but if we are not planning to support remote case yet, I think ipc is simpler.
and when ipc is used, client and server is on the same machine so we don't need to worry about client/server on different OSes and etc.
2 participants
Teach the server to advertise and manage extra read-only channels for new TSP capabilities.