Skip to content
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

Adding experimental support for GRPC for the extensions to communicate with CSharpier #944

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

belav
Copy link
Owner

@belav belav commented Aug 26, 2023

No description provided.

@shocklateboy92
Copy link
Collaborator

So, is there a reason you want GRPC specifically, other than a learning experience?
If you create a server that listens on an TCP port, you have to deal with a fair bit of extra concurrency issues.
It would be much simpler if you just pipe the protocol buffers into stdin/stdout like you did with the plain text earlier.

And of course, since our proto file contains a single string, we might get away with just using JSON like other all the language servers seem to do.

PS: If the primary reason for using GRPC specifically was for a learning experience, would you consider this PR as having given that experience? Or do you want to merge it into master and ship it?

@belav
Copy link
Owner Author

belav commented Sep 3, 2023

It would be much simpler if you just pipe the protocol buffers into stdin/stdout like you did with the plain text earlier.

This seems like it would require the same thing as CSharpier currently does, which is looking for a specific character/byte to know when a request to format a file has been received. I really just starting googling protobuf which lead me to grpc and the initial POC for just the c# side of things was super easy.

If you create a server that listens on an TCP port, you have to deal with a fair bit of extra concurrency issues.

I'm hoping that all of these issues are hidden from me from using the grpc library. I also wanted to try out named pipes, which seems like the "proper" way to do IPC. But I don't have a clear answer on if it works with linux or not.

The code in the POC so far seems quite a bit simpler than running a process ourselves, I'm gonna play around a bit more because it is motivating right now. I am worried about what happens when you have multiple IDEs running, and you need to use a dynamic port or a dynamic named pipe. I was also having issues connecting to the server when running csharpier via a regular node child process which lead me to using vscode tasks, which may be the proper way to start up the csharpier process. Although it is unclear if you can pipe stdin/out with them.

TLDR - having fun learning for now, none of this may actually make it into the real code base.

@belav
Copy link
Owner Author

belav commented Sep 3, 2023

The proto version doesn't do everything yet, but it does look way more straightforward right now

https://github.com/belav/csharpier/blob/proto/Src/CSharpier.Cli/ProtoFormatter.cs
vs
https://github.com/belav/csharpier/blob/proto/Src/CSharpier.Cli/PipingFormatter.cs

@belav belav force-pushed the proto branch 2 times, most recently from 0f3e397 to ecb272c Compare September 3, 2023 18:15
@belav belav force-pushed the proto branch 2 times, most recently from 07bcf64 to eb5d0ed Compare December 31, 2023 19:12
@belav belav mentioned this pull request Dec 31, 2023
@belav belav changed the title POC for grpc Adding experimental support for GRPC for the extensions to communicate with CSharpier Dec 31, 2023
@belav belav marked this pull request as ready for review December 31, 2023 19:24
@belav
Copy link
Owner Author

belav commented Dec 31, 2023

@shocklateboy92 I think this is ready to be considered experimental. It works well from what I can tell in VS & VSCode, although I don't plan on releasing updated extensions for them just yet.

It isn't clear why with VSCode using spawn results in not being able to connect to the GRPC server, I assume it is some kind of permissions issue. And using a task results in not being able to get anything from stdout which resulted it needing to find a free port on the VSCode side.

@belav belav added this to the 0.27.0 milestone Dec 31, 2023
@shocklateboy92
Copy link
Collaborator

Cheers. I'm kind of busy at the moment and this is a big PR.

I'll get to it next week if that's alright.

@belav
Copy link
Owner Author

belav commented Jan 2, 2024

I'll get to it next week if that's alright.

Not a problem at all! I wasn't sure if you saw when PRs switched away from draft, and I knew you had some concerns with this one originally so wanted to make sure you saw it.

@belav belav modified the milestones: 0.27.0, Planned Jan 2, 2024
@belav belav force-pushed the proto branch 2 times, most recently from 6eac39f to d6913a0 Compare January 15, 2024 17:01
@belav belav merged commit 99206da into main Jan 15, 2024
3 checks passed
@belav belav deleted the proto branch January 15, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants