Server doesn't respond to invalid request

[robstryker]

A launcher created via the following API call:

		this.launcher = Launcher.createLauncher(localService, remoteInterface, socket.getInputStream(), socket.getOutputStream());

Will not respond to an invalid http request which lacks the content-length attribute, or any other malformed request. An error is logged on the server, however, no response is sent to the client.

Jun 07, 2018 1:02:55 PM org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer fireError
--
SEVERE: Missing header Content-Length in input "GET / HTTP/1.1
Sec-WebSocket-Version: 13
Sec-WebSocket-Key: 4vrbTo0+BlSk5wK7si6SWQ==
Connection: Upgrade
Upgrade: websocket
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits
Host: localhost:27511
 
"
java.lang.IllegalStateException: Missing header Content-Length in input "GET / HTTP/1.1
Sec-WebSocket-Version: 13
Sec-WebSocket-Key: 4vrbTo0+BlSk5wK7si6SWQ==
Connection: Upgrade
Upgrade: websocket
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits
Host: localhost:27511
 
"
at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.listen(StreamMessageProducer.java:89)
at org.eclipse.lsp4j.jsonrpc.json.ConcurrentMessageProcessor.run(ConcurrentMessageProcessor.java:95)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)

The StreamMessageProducer is created through a series of static methods stemming from the Launcher.createLauncher call, and so the creator of this launcher has no opportunity to customize the error handling. However, even in the base case without an extender's customizations, I believe the server should respond to these invalid requests with something like a code 400 http response.

To replicate, you can start an LSP using lsp4j + json, and send the following to the port via telnet:

GET / HTTP/1.1
--
Sec-WebSocket-Version: 13
Sec-WebSocket-Key: 4vrbTo0+BlSk5wK7si6SWQ==
Connection: Upgrade
Upgrade: websocket
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits
Host: localhost:27511

The server should respond something, even if only an error code, but it does not. It simply logs the error on the server side and does not respond to the client.

[jonahgraham]

This looks essentially the same underlying issue as #189. The lps4j implementation is not an implementation of an HTTP transport (also the issue raised in #203 is related as that is also about transport). All three of these issues require a way to split the transport from the message handling part of the code. It is worth having a look at microsoft/language-server-protocol#86 too as a cross reference.

[mickaelistria]

I agree with @jonahgraham . This issue is not LSP4J's fault and should probably be closed.

LSP4J expects the input/output stream to talk the LSP language directly, with everything it requires and without noise such as unrelated headers. So what you should do is to create a stream derived from your SocketStream that reads the input, trims out the headers, adds the Content-Length and forwards the LSP payload to the stream passed to the launcher.
It's basically implementing a FilterOutputStream that removes irrelevant header parts and adds the Content-Lenght parts. If this happens to be plain Java (no strong dependencies), it would be nice to consider contributing this WebSocketToLSPFilterStream to LSP4J as a utility for other people sharing the same issues.

[robstryker]

I'm not complaining about the fact that lsp4j blows up when there's an error or a malformed request. I'm also not looking for an API to manually massage the client's request to make it work for lsp4j, which I think is stupid. I'm focusing exclusively on error-handling here, or rather the lack-thereof.

There should exist some API where we get to respond to a given request that fails to be parsed correctly with any number of http status codes and messages. This API should exist. It should be easy for a developer to access. And it should work.

Currently the lsp4j server just swallows all errors and returns nothing to the client. The client has no real API to return various status codes either.

If lsp4j intends to treat the lack of a content-length as an error, then shouldn't it also do out.consume(errorResponse)?

Even returning a raw stacktrace or the ResponseError response would be more appropriate than simply swallowing the error in its entirety and responding nothing. I can't imagine any scenario where responding nothing is even remotely appropriate in the least. An extender of LSP4j should not be required to inject itself into the streams, reading them and piping them along to the actual lsp4j implementation just to make sure the server doesn't respond nothing to a malformed request. This should be built in to the implementation already.

[jonahgraham]

Hi Rob,

Not sure I fully understand what you are asking. But it sounds like you want LSP4J to behave like an HTTP server, rather than like an LSP implementation? LSP defines only two headers, and is defined as Comparable to HTTP, but it is not HTTP. If LSP4J started sending http error codes back then LSP4J would not be implementing the spec.

LSP4J does send back the errors that are defined by the spec, you can see the tests for some of them, such as malformed JSON which has a response according to the spec here.

Is it possible the correct place for the issue you want to raise is against the language server protocol specification itself?

Currently the lsp4j server just swallows all errors and returns nothing to the client. The client has no real API to return various status codes either.

If lsp4j intends to treat the lack of a content-length as an error, then shouldn't it also do out.consume(errorResponse)?

Have a look at the versatility test for some examples of how clients can send errors back to the server.

I think you understood this, but my previous response was if you want to use another transport than what the LSP defines, then LSP4J should make APIs available to translate between such transports more easily than doing it at the stream level.

HTH,
Jonah

[robstryker]

The LSP specification includes this text:

A request that got canceled still needs to return from the server and send a response back. It can not be left open / hanging. This is in line with the JSON RPC protocol that requires that every request sends a response back.

The JSON RPC spec includes the following:

5 Response object

When a rpc call is made, the Server MUST reply with a Response, except for in the case of Notifications. The Response is expressed as a single JSON Object, with the following members:

I am demonstrating for you right here that there exists a usecase where a request receives no response at all, thus violating both the LSP spec and the JSON RPC spec.

Am I incorrect here? There exists a request that gets no response. Both specs say that's bad. LSP4j received a request. It didn't like the absence of content-length. It chose not to respond. This violates the spec, no?

[cdietrich]

that is what i observed with TypeFox/vscode-ws-jsonrpc#6 and what lead to opening #189

so here i am not sure how to deal with that:

• either the header is treated as part of the protocol but why then does the vscode-ws-jsonrpc not send it
• or it is not but consider as part of the protocol.

imho it is part of the protocol and clients that dont send it are bad clients but anton seems to see that differently (see the TypeFox/vscode-ws-jsonrpc#6 issue)

[robstryker]

Even if the clients are bad clients, they deserve SOME RESPONSE telling them they are bad clients. Probably a ResponseError or something. I can't imagine ANY world where simply ignoring the request is a-ok.

[cdietrich]

but if client and server cant agree to a protocol how can they send error messages.
sets asume the clients expects a protocol that terminates every message with 0A0A0A but the server never knows, how shall it send a error message. it only can try to emulate the message behaviour it expects so send a regular lsp message response. but that wont help cause in the websocket case the client cannot handle such responses too.

[robstryker]

I see your point, but then I'd expect the server to respond in its own protocol with an error message. If the client doesn't know what it means, that's the bad-client's fault. If I issue a nonsensical bunch of garbage to google.com:80, google will eventually return me an http error message.

For example:

 telnet google.com 80
Trying 173.194.219.113...
Connected to google.com.
Escape character is '^]'.
blah de blah sdfhsdfbhsd f

HTTP/1.0 400 Bad Request

In the LSP case I would expect the LSP to respond with a proper LSP-protocol error string, even if it receives an absolute bunch of garbage.

If on the other hand I telnet to an lsp4j implementation and do the same, I get no response at all.

Again I see your point on if the server can't tell the message is complete or not. But for this specific content-length issue, I would imagine we could send back an LSP error message here rather than swallow and ignore the request. Maybe I'm just plain wrong though.

[mickaelistria]

To be honest, the issue you're facing is a dev-time issue, and shouldn't happen when development is complete and correct, so shouldn't happen as usage. Error handling is important at usage, less important at dev-time if those are errors users wouldn't see.
As a result, I think what you're asking for here would be a nice to have, I don't think it's really a major severity.

There should exist some API where we get to respond to a given request that fails to be parsed correctly with any number of http status codes and messages. This API should exist. It should be easy for a developer to access. And it should work.

This is not part of the spec AFAIK and probably more something to discuss on the LSP side than in LSP4J directly.

Even returning a raw stacktrace or the ResponseError response would be more appropriate than simply swallowing the error in its entirety and responding nothing.

That's seems like an interesting proposal. But what about erroneous notifications then, where nothing is expected and ResponseError is not likely to occur? Also, when a LS sends an erroneous header at some point, how can we be sure which request did trigger the invalid header and which Request should be answered a response error?
I think it's the whole purpose of using a JSON-RPC + LSP spec: just stick to it and everything is fine; don't stick to it and face doubt, issues, trouble, hate... Forcing clients to stick to the spec is better than allowing erroneous content. Especially if we don't have any guarantee other clients would work as well with the same content. What you're asking for would make LSP Servers/Clients developed by LSP4J support case that are erroneous according to the specification, introducing a risk of the implementation not being compatible with other clients/servers not using same tolerance to erroneous content.
Overall, once again, I don't think it's something that should be debated and implemented in LSP4J before the spec gives advice on implementation. So maybe this should be brought as an issue to the spec first.

I am demonstrating for you right here that there exists a usecase where a request receives no response at all, thus violating both the LSP spec and the JSON RPC spec.

That's a fallacious demonstration: the message that was sent to the LS in that case isn't a proper rpc call in the definition of the LSP spec as it misses a required header. So if it's not a proper rpc call, the part of the spec you mention doesn't apply and there is no reason to guarantee it gets a response.
I think you've mostly demonstrated an inconsistency in the LSP spec regarding headers: it says it's JSON-RPC which doesn't mention a Content-Length header is required while LSP says that Content-Length is required. (Again) This case needs to be clarified in the LSP spec before LSP4J implements anything. As a consumer of LSP, I would be in favor of making the Content-Length optional in LSP spec.

For example: [...] In the LSP case I would expect the LSP to respond with a proper LSP-protocol error string, even if it receives an absolute bunch of garbage.

I think this case isn't well specified and that your expectation isn't completely consolidated by the spec (so, guess what I'm going to say, this should be discssed on the spec side ;).
Have you checked how non LSP4J-based LSP clients/server (like VSCode and its CSS Language Server) do behave in that case?

Summary:

• It's a spec inconsistency or lack of clarity, to fix in the spec first
• Just hack your streams to have a Content-Length and no extra headers and you won't have any issue nor doubt about conforming to the spec or not ;)

[robstryker]

Just hack your streams to have a Content-Length and no extra headers and you won't have any issue nor doubt about conforming to the spec or not ;)

Why not make lsp4j just do this automatically then? Why should each and every LSP implementation have to hack their streams, when you could just make it part of lsp4j directly?

If we're noticing that json libraries in a variety of languages do or do not include content length, and that this is a common problem in multiple clients over multiple languages, why wouldn't lsp4j just do this automatically? That's the part I don't get.

Why should I ever have to hack my streams? YOU should have to hack the streams, not me.

[jonahgraham]

How is LSP4J supposed to know the content length? What is the framing being used? LSP defines the framing by the content length.

[jonahgraham]

Why should I ever have to hack my streams?

The simple answer is you have to hack your streams when your stream doesn't match the spec.

I feel we are having a little XY problem here. What is your use case? Why isn't your stream already an LSP stream if you want to do LSP stuff? It sounds like you are implementing a client?

I would really like to help as I am still keen on making LSP4J as usable in different contexts as possible.

Thanks,
Jonah

[robstryker]

Hi Jonah:

Yeah there does seem to be a little XY problem here ;) I am implementing the server, based on lsp4j. It's actually not an LSP, but something based on the LSP base protocol. Imagine the chat app made by typefox for example (https://github.com/TypeFox/lsp4j-chat-app). It uses all the same base-protocol stuff, like Message, RequestMessage, ResponseMessage, ResponseError, etc etc etc.

Some others on my team are implementing the client, and they basically opened a bug to me complaining that a request missing the content-length responds with nothing. This is where you come up with the statement that the stream is not an LSP stream. If it's missing its content-length, then it doesn't fit the spec. So the real question is, why did they send a message without a content-length, and why did they expect it to work?

After doing some reading, I think i understand now your distinction between the transport and the protocol, and I also agree that a content-length is probably pretty important to keep around.

However I still believe that the lsp protocol itself, since content-length is required, should find a way to send some type of error back if that header is missing, rather than just leaving the client 'hanging'. This seems to be a spec issue, since the spec doesn't say what should happen if content-length is missing. You're right about that.

To revisit why they sent me a bad stream and expected it to work, I actually don't know the answer. What I do know is what error they included in the jira bug:

java.lang.IllegalStateException: Missing header Content-Length in input "GET / HTTP/1.1
--
Sec-WebSocket-Version: 13
Sec-WebSocket-Key: 4vrbTo0+BlSk5wK7si6SWQ==
Connection: Upgrade
Upgrade: websocket
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits
Host: localhost:27511
 
"
at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.listen(StreamMessageProducer.java:89)
at org.eclipse.lsp4j.jsonrpc.json.ConcurrentMessageProcessor.run(ConcurrentMessageProcessor.java:95)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)

So I think we can assume they're using websockets, and that this is also a development-time bug that, once they start sending me proper streams, will disappear.

If I had to distill my complaint down, I'd suggest the following few items:

1. content-length should be explicitly declared as required or optional. The spec seems vague to me.
2. I still believe (within reason) that every request deserves a response. The missing header should still lead to the remainder of the stream being parsed until the end of the message can be discovered, perhaps by counting curly braces or something, the message dropped, and some type of ResponseError should be sent back.
3. I would prefer if the spec defined what the behavior should be if content-length is missing, since this seems to be a pretty common error (judging by how much debate people have had over it).

I hope that helps to clarify the situation. Thanks again for your help. Any advice will be much appreciated.

[cdietrich]

thats the very same problem i head. and i was told it should be fixed on lsp4j side. i had the opposite view initially. this is why i opened TypeFox/vscode-ws-jsonrpc#6 first.

[jonahgraham]

Since this issues was last discussed websocket support was added as an extra bundle/project. If there is still something that can/should be done here please reopen and I'll add a help_wanted label

[georghinkel]

Sorry for asking this question, but what makes you think that a missing Content-Length header in the web socket creation makes it invalid? I can't find that requirement anywhere in the specs of either JSON-RPC 2.0 (which are very small) or web sockets. Actually, you guys should be fine if that header is not present or am I missing something?

[szarnekow]

Sorry for asking this question, but what makes you think that a missing Content-Length header in the web socket creation makes it invalid?

It's mandatory in LSP: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#headerPart

[georghinkel]

@szarnekow Thanks for the reference, but that only affects the headers sent with the web socket payload, is it? The requirement does not apply to the web socket handshake and that is what caused the actual issue. I see why the Content-Length is required for the payload as a TCP connection does not offer option to see boundaries of messages, but the HTTP handshake to create the web socket does not have any content and therefore should not require a Content-Length.

[georghinkel]

So in my case, the misunderstanding got cleared. I was expecting the server to offer a web socket where the server ran the JSON-RPC protocol over a plain TCP socket. The suggested message to replicate the bug in the original post seems to imply the same misunderstanding because this is exactly a web socket connection request. If the server in question runs LSP over a plain TCP socket, it must try to understand this web socket handshake as a LSP message and hence cause this error.

[cdietrich]

side note: maybe https://github.com/TypeFox/monaco-languageclient/tree/main/packages/vscode-ws-jsonrpc might be for interest of you