Improve handling of unexpected errors

[tombh]

This jumps off from an issue identified by @alcarney in the v1 breaking changes PR #273 (comment)

Pygls does already catch some errors, but not all. Nor does it offer a way for server authors to handle those unexpected errors.

@alcarney Is that a fair summarisation of the problem? Obviously the underlying message parsing problem needs to be fixed, but is it not a reflection of a deeper issue, that Pygls couldn't recover from that error?

Recently I've been attempting to build a Pygls template project, that offers a reasonable and informative starting point for new custom servers. Maybe it's my relative inexperience with Pygls, but I noticed that there doesn't seem to be a centralised point for all error handling? My naive approach is to decorate the feature decorator like so:

def add_feature(self, feature: str):
    """
    This is a wrapper just to catch and handle all unexpected errors in one place.
    TODO:
    Is this even useful? I wonder if this should be formally supported in Pygls itself?
    """

    def wrapper(func):
        @self.feature(feature)
        async def inner(*args, **kwargs):
            try:
                await func(*args, **kwargs)
            except Exception as e:
                self.logger.error(e)
                # TODO: should this have the name of the custom LSP server?
                self.show_message(f"Unexpected error in LSP server: {e}")

        return inner

    return wrapper

Does that seem relevant to the deeper issue at hand?

[alcarney]

To be explicit about the error I'm facing, here is where pygls parses incoming messages and any exceptions thrown by _deserialize_message are currently unhandled taking the server down with it.

Now, I could be mistaken, but I don't think there's much handling we can do here by default other than catching and dropping the error (the reasons for the error have already been logged by _deserialize_message). As far as I know the only space for sending an actual error message to the client is when the server receives a request, so 3/4 of the time there is no standard way to tell the client about the error.

but I noticed that there doesn't seem to be a centralised point for all error handling?

I think you're right... and having the ability to register an error handler could be really useful. Certainly for pytest-lsp I want to be able to catch these malformed rpc messages and fail the test (Between all the moving parts at the moment I'm having so many issues with hanging tests! 😅 )

I'm not sure however, how useful wrapping user features will be... I fairly sure issues in features are already caught by the existing error handling in JsonRPCProtocol?

[tombh]

So what we're saying is that JsonRPCProtocol is the deepest part of Pygls where server lifecycle errors can be caught? And that it already has a try/catch block, but that it could be "deeper"? Yeah, catching and dropping errors is the most that can be expected, sending them on to the client is just a bonus. Basically, I think it's clear that Pygls should be able to survive a broader range of errors. Or put another way, what's the most reasonable depth at which a try/catch block can be placed such that, at the very least, if the server boots it doesn't then crash from any general lifecycle errors. Anyway, tl;dr my feeling is that the current base try/catch block is not at the deepest place it could be, right?

As far as I know the only space for sending an actual error message to the client is when the server receives a request

Oh, my understanding was that LSP's showMessage.type = MessageType.Error was the standard way of telling the client about errors? And that showMessage is unsolicited, as in, it can be sent outside of request/response lifecycles? That understanding is from reading the spec, not from Pygls' implementation. Hence why I'm wondering about how we can either formally support error handling in Pygls itself, or at least offer a best practice means for authors to do it themselves.

Yes I had an annoying test-hang bug the other day too! The main thing ended up being having to add a timeout in a subprocess call, so it wasn't either Pygls' or pytest-lsp's fault. But it did force me to better set up logging in the server, because of course, by default the client, which is the only thing pytest has access to, doesn't receive much in the way of feedback and logging from the server running in another thread.

Anyway, wrapping @server.feature was just my naive approach. Hooking into wherever the deepest try/catch is would be the best approach. But from what you've said, and my perhaps still naive understanding of the showMessage spec, I'm now leaning towards defaulting to sending showMessage.type = MessageType.Error messages to the client. But offering an easy way to override that default behaviour, maybe another decorator?

[alcarney]

So what we're saying is that JsonRPCProtocol is the deepest part of Pygls where server lifecycle errors can be caught?

Yes, everything winds up coming through this class at some point. If you're interested, messages come into data_received and responses eventually find their way into _send_data.

And that it already has a try/catch block, but that it could be "deeper"?

Not deeper... but we need another one to catch some errors in data_received that happen right at the start of the lifecycle

Oh, my understanding was that LSP's showMessage.type = MessageType.Error was the standard way of telling the client about errors?

🤦 you're right! My head is so deep in JSON-RPC land at the moment I forget about window/showMessage :D

I'm now leaning towards defaulting to sending showMessage.type = MessageType.Error messages to the client.

Sounds good 😄

[tombh]

Ok great, thanks for the insights 🤓 So just to sketch things out:

• data_received, like _send_data, should have a try/catch block.
• Concerning notification errors, both should have an easily configurable (a feature-like decorator or something) shared custom function in their except: blocks.
• By default that shared function should send a showMessage.type = MessageType.Error message to the client on errors triggered by notification responses.

Not saying you should do all that if you're the first in there. Just making a note for the future.

[tombh]

Oh, wait! Hold the press! I've figured something out: errors in LSP requests and notifications are handled differently.

So, the actual formal way to handle a LSP client request is with a LSP ResponseError in ResponseMessage.error. Which we do already do in JsonRPCProtocol._handle_request() ✅

Now, here's the thing. LSP requests and notifications are 2 distinct things. You probably already knew that, but I've only just realised that now 😬 For anybody else that isn't aware of the difference, textDocument/didOpen is an example of a notification, whereas textDocument/completion is a request. And in Pygls' case, JsonRPCProtocol._handle_notification() does not attempt to return any errors to the client. It does however at least catch and locally log errors.

So, this changes gears a little. Unchanged is that we're still wanting to improve error handling for data_received. But more specifically we want to default to sending showMessage.type = MessageType.Error messages to the client for notifications only. With a convenient way to override that behaviour.

I've updated my previous comment to better reflect this new insight.