Stream read errors result in uncaught exceptions

[hansonw]

Currently, message errors from StreamMessageReader's onData method end up as uncaught exceptions, which can be very difficult to handle when used in server contexts.

https://github.com/Microsoft/vscode-languageserver-node/blob/master/jsonrpc/src/messageReader.ts#L220

Would it be reasonable to convert such errors into error events (via this.fireError) so that the stream owner can perform shutdown / restart steps as necessary? I can submit a PR to do so. cc @ljw1004

[dbaeumer]

Yes, but we should clearly defined that the readere then moves into an error state and doesn't process any new onData events since it can not meaningful recover from a missing content length.

[raxod502]

Any workaround? It is really really bad if your server crashes just because some LSP stream got broken. Depending on context, could be a security vulnerability.

[dbaeumer]

The got improved already an errors are now reported on the connection. What esle do you expect the reader to do if it receives a broken message

[raxod502]

When an error occurs, the entire Node.js process is crashed for me. Do I have to register an error handler to prevent this from happening, or am I doing something else wrong that is causing me to mistakenly blame this package?

[dbaeumer]

Not sure I understand that correctly. The server node process crashes ?

[raxod502]

Yes. The Node.js process that is running vscode-languageserver-node crashes (after printing stack trace from vscode-languageserver-node).

[dbaeumer]

What seems strange to me is that the node process crashes. It shouldn't :-)

But I am open for a PR that converts the exception into a fireError.

[raxod502]

In particular this is what is printed before the server crashes.

/home/docker/src/node_modules/vscode-jsonrpc/lib/messageReader.js:163
                    throw new Error('Header must provide a Content-Length property.');
                    ^

Error: Header must provide a Content-Length property.
    at StreamMessageReader.onData (/home/docker/src/node_modules/vscode-jsonrpc/lib/messageReader.js:163:27)
    at Socket.<anonymous> (/home/docker/src/node_modules/vscode-jsonrpc/lib/messageReader.js:148:18)
    at Socket.emit (events.js:314:20)
    at addChunk (_stream_readable.js:303:12)
    at readableAddChunk (_stream_readable.js:279:9)
    at Socket.Readable.push (_stream_readable.js:218:10)
    at Pipe.onStreamRead (internal/stream_base_commons.js:188:23)

[raxod502]

What seems strange to me is that the node process crashes. It shouldn't :-)

I just tested, and uncaught exceptions do (in general) crash Node. I believe this is intended behavior. Test:

% node -e "setTimeout(() => { throw new Error('foo') }, 1000); setTimeout(() => console.log('done'), 2000)"
[eval]:1
setTimeout(() => { throw new Error('foo') }, 1000); setTimeout(() => console.log('done'), 2000)
                   ^

Error: foo
    at Timeout._onTimeout ([eval]:1:26)
    at listOnTimeout (internal/timers.js:551:17)
    at processTimers (internal/timers.js:494:7)

[dbaeumer]

OK. Now I understand. Under crash I was assuming a real process crash.

For the servers I maintain I have a process.on('uncaughtException', (error: any) => { ... }) handler since exception can occur in other parts of your server as well.

[dbaeumer]

This got addressed in the meantime. The exception are now reported via the onError event.