How serial should notifications be?

[Desdaemon]

This is a continuation of ebkalderon/tower-lsp#284, but personally it would be more suitable as a discussion topic if we do enable them.

One of the reasons async-lsp was made was to resolve the pain point of concurrent handling not respecting the expected order of execution, as outlined in the specs here with surprisingly vague language:

Responses to requests should be sent in roughly the same order as the requests appear on the server or client side. [...] However, the server may decide to use a parallel execution strategy and may wish to return responses in a different order than the requests were received. The server may do so as long as this reordering doesn’t affect the correctness of the responses.

While this was never completely solved in tower-lsp, what I found instead was that when all methods can be concurrent w.r.t each other, the responsibility of keeping certain requests serial to each other was pushed onto the server developer. For example, this is a test case I was writing in an attempt to verify that notifications should be serial w.r.t some requests:

#[tokio::test(flavor = "multi_thread")]
async fn notification_order_preserved() {
    let reqs = [
        REQUEST, // initialize
        r#"{"jsonrpc":"2.0","method":"initialized","id":2}"#,
        // all requests and notifications should be allowed after this
        // assuming all requests are for the same file

        // assuming `hover` and `did_open` are for the same file, what should `hover` return?
        r#"{"jsonrpc":"2.0","method":"hover","id":3}"#,
        r#"{"jsonrpc":"2.0","method":"did_open","id":4}"#,
        
        // `complete` could finish before `did_open`, even though it was logically sent after
        r#"{"jsonrpc":"2.0","method":"complete","id":5}"#,

        // but `did_change` should DEFINITELY be run after AND finish after `did_open`
        r#"{"jsonrpc":"2.0","method":"complete","id":6}"#,
    ];

    let stdin = ..;
    let (mut stdin, mut stdout) = (Cursor::new(stdin), Vec::new());

    Server::new(&mut stdin, &mut stdout, MockLoopback(vec![]))
        .serve(TestRequestOrderService) // returns a canned response with the id copied
        .await;

    // do we expect a fully serial response?
    let response = concat!(
        "Content-Length: 38\r\n\r\n",
        r#"{"jsonrpc":"2.0","result":null,"id":1}"#,
        "Content-Length: 38\r\n\r\n",
        r#"{"jsonrpc":"2.0","result":null,"id":2}"#,
        "Content-Length: 38\r\n\r\n",
        r#"{"jsonrpc":"2.0","result":null,"id":3}"#,
        "Content-Length: 38\r\n\r\n",
        r#"{"jsonrpc":"2.0","result":null,"id":4}"#,
        "Content-Length: 38\r\n\r\n",
        r#"{"jsonrpc":"2.0","result":null,"id":5}"#,
        "Content-Length: 38\r\n\r\n",
        r#"{"jsonrpc":"2.0","result":null,"id":6}"#,
    );
    assert_eq!(std::str::from_utf8(&stdout).unwrap(), response);
}

Whether hover and did_open should finish before or after did_open is immaterial, because for correctness' sake they must verify if did_open was in fact run for the file, and thus generated the necessary context. They cannot assume the order of operations, because that would be the same as depending on the implementation details of the client.

What is less controversial, however, is that mutations to a file should be witnessed in the same order between the client and the server, as in the above example where we want did_change to always be run after did_open. In other words, even if the user yields from a future returned by did_open, it cannot begin execution of did_change since did_change would not have the proper context to handle the notification.

However, as with everything, I believe implementations in practice would have to handle file states (usually the file's version in the specs) in order to maintain correctness, if they cannot depend on the framework (tower-lsp) or force themselves to handle notifications synchronously (async-lsp).

I'm also concerned with whether long-running requests that don't mutate file, but do mutate context, should still be somewhat serial w.r.t mutating notifications. If a request took too long and a did_change notification already came, the request would continue but now with out-of-date information, and now they have to decide if they want to toss all the context after every did_change, deal with cache invalidation, so on and so forth. What would be the appropriate guarantees for tower-lsp-server to make, in response to these challenges?

Lastly, I do not think async-lsp's way of handling notifications is the silver bullet either, as many tasks done by both requests and notifications could be async in nature, and if a user wants to do async stuff in notifications they'd have to create their own task queue and consume it manually as well. Or rely on tokio::spawn or std::thread::spawn as much as possible.