WorkDoneProgress tokens not initialized properly by the server

[rchl]

1.7.0 introduced progress reporting support but this doesn't work in Sublime Text for me and I'm pretty sure that's because it's not implemented correctly.

The $/progress notifications are being sent with random token that hasn't been created beforehand using the window/workDoneProgress/create request.

As far as my understanding of the spec goes, the token used in $/progress notification has to be first explicitly created by the server. Refer to https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initiatingWorkDoneProgress and https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#serverInitiatedProgress.

[ccordoba12]

Hey @rchl, thanks for reporting. If possible, could you submit a PR with your suggested fixes? I think it'd be easier for you given that you understand the situation better.

[syphar]

@rchl thank you for the info!

Do you known if sublime would actually initialize client side progress given we return the capability?

With my editor everything worked as-is, also the other language server impl I checked doesn't initialize the progress as it is documented. Which is why I missed this piece.

I'll be able to dig deeper on this today, or the next days.

[syphar]

I'll also check if my editor (neovim) would create and provide the token so the progress reporting is initialized client side.

[rchl]

ST LSP does initialize client side progress for some requests but not all. It does for those that intend to run longer like textDocument/documentSymbol, workspace/symbol or all the variants of the "go to ..." requests. But it can't be expected that all requests will have progress initialized on the client side.

[syphar]

So the (design-) question is if we should adhere to the client side progress initialization for requests that are triggered by the client. So we would actually not log any progress when the client side initialization is missing.

[syphar]

I just checked some other LSP servers:

• rust-analyzer uses server-side initialization of the progress, though it also only reports progress in the initialization phase. It doesn't report workDoneProgress as server capability for the providers, but also doesn't report progress on them.
• null-ls doesn't initialize the progress token in either way, also doesn't report the workDoneProgress capability
• sumneko-lua uses server-side initialization of progress, also reports progress for diagnostics & formatting. they have a hardcoded minimum delay when to show the progress, sometimes, 0.5-1 seconds, depending on the place. Also it doesn't report the workDoneProgress server capability.

Seeing this, I lean towards also using server-side initialization to fix this.

Any minimum delay would be a separate issue / PR IMO.

[rchl]

It gets a bit confusing the more I look at it...

Let's say the server adds workDoneProgress capability to all relevant providers. Assuming that the client supports progress reporting for that request, should it always initialize a progress token and add it as a workDoneToken field to the request? At least for ST it's not the case right now. It only chooses to initialize progress token for those few requests mentioned above.

But lets say the client would do that and always initialize progress token if server reports workDoneProgress capability for given provider. Then it would be still up to the server to decide whether it wants to report progress or not for given request. There can also be a case that the client supports progress reporting but doesn't initialize the client-side token. Then the server could still initialize the token itself.

In any case, I think we have clarity on how to handle progress reporting on the LSP side but what about the subjective aspect of the issue of whether to report progress for certain actions (like separate progress for each linter) or not? Is that something you plan to address as part of your incoming changes or are you just focusing on handling progress tokens correctly?

[rchl]

Seeing this, I lean towards also using server-side initialization to fix this.

And if you want to report progress for individual linters then of course you have no choice here as those are not part of any request and can't use client-side initialized token.

[rwols]

Let's disambiguate some notions so we're all on the same page here.

There is client-initiated progress and there is server-initiated progress.

The current implementation in python-lsp-server is doing server-initiated progress, because it creates its own tokens:

python-lsp-server/pylsp/workspace.py

Line 154 in 2b3ac73

token = str(uuid.uuid4())

and reports progress based on the workspace, not some particular client request. So you can call the construct

with workspace.report_progress("my message"):
    ...

literally anywhere without regard of a client request.

The "bug" is that according to the spec, the server must request window/workDoneProgress/create to the client and await its response before reporting progress.

Unfortunately, the with workspace.report_progress("...") construct is blocking; it would have to accomodate the request to the client first. One way to do that would be to make it async:

async with workspace.report_progress("my message"):
    ...

The same "bug" is present in Ecliplse JDT LS: eclipse-jdtls/eclipse.jdt.ls#2377, sublimelsp/LSP-jdtls#37

Given that two servers don't request window/workDoneProgress/create I'm somewhat inclined on just ignoring that request client-side because we don't really need any preparation for a progress monitor.

The point of window/workDoneProgress/create is for the client to be able to set up a progress monitor in its UI like a progress bar from what I understand.

[rwols]

Having said all that, for handling client requests you should look into client-initiated progress which is not (yet) implemented by this server. In that case you don't have to generate the token yourself with uuid.uuid4() but rather use the provided token in the workDoneProgress field of the request data to start reporting progress. As a bonus, you don't have to call and await the window/workDoneProgress/create request.

[rchl]

Note the above comment though: #325 (comment)

[syphar]

short update:
I started working on doing (and waiting for) window/workDoneProgress/create, which already works with my editor.

As usual, writing the tests is tricky, I'll create a PR when that works.