ebkalderon / tower-lsp

Language Server Protocol implementation written in Rust
Apache License 2.0
962 stars 54 forks source link

TCP examples open ports; they should connect instead #334

Closed jmcphers closed 2 years ago

jmcphers commented 2 years ago

I've been trying my hand at creating an LSP using this project. Very early stages, but having this framework is so helpful!

One problem I had early on was dealing with a port conflict: both my LSP client and the LSP server (built with Tower) wanted to listen on the port, but of course only one process can listen to any particular port. I was pretty confused until I ran across this comment:

https://github.com/microsoft/language-server-protocol/issues/604#issuecomment-439853638

What needs to be understood is that for connection timing issues the server is actually a client and the client is the server in terms of opening the ports.

When you specify a socket transport the client is listening on that port for a connection.

So, this code (https://github.com/ebkalderon/tower-lsp/blob/6e7af0e13561140df17c41e68c53b90223333158/examples/tcp.rs#L126-L128) doesn't work, because the client is already listening to the port:

    let listener = TcpListener::bind(format!("127.0.0.1:{}", port))
        .await
        .unwrap();
    let (stream, _) = listener.accept().await.unwrap();

Here is the code I'm using instead, which works and connects to the client:

    let stream = TcpStream::connect(format!("127.0.0.1:{}", port))
        .await
        .unwrap();
    let (read, write) = tokio::io::split(stream);

I don't understand why the examples listen on a port instead of connecting to one -- am I missing something? If not, would be happy to submit a PR to update the examples.

ebkalderon commented 2 years ago

That looks correct to me! I think the original examples had overlooked these semantics. I'd be happy to accept a PR that switches the tcp.rs example to use TcpStream::connect(), if you'd like to open one?

jmcphers commented 2 years ago

Great, thanks for clarifying! Will do.

ebkalderon commented 2 years ago

So, the TcpListener listens on a port indefinitely and generates TcpStream instances as clients connect to it, which technically makes sense for a remote server. But considering that LSP only supports one client connection at a time, and we never actually reuse the TcpListener for instantiating new TcpStreams, as is intended, I think it makes perfect sense to use TcpStream::connect() here instead, like you suggest.

We'd still have to test the new examples/tcp.rs server against an actual client to make sure the new connection method behaves as expected, of course.

jmcphers commented 2 years ago

FWIW I did test this from VS Code and got an established TCP connection:

[2022-04-15T22:40:12Z TRACE tower_lsp::codec] <- {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":86932,"clientInfo":{"name":"Visual Studio Code","version":"1.65.2"}, ...
ebkalderon commented 2 years ago

Awesome! Seems like it works as expected.