Fixes for work done progress

[perrinjerome]

Description (e.g. "Related to ...", etc.)

Work done progress ( relevant specifications for reference ) had a few problems. It was possible for server to create a cancellable progress, but cancellation message were not handled by pygls framework. There was an implementation of cancel, but it was incorrect, as pointed out in #233 , it was implemented as if cancel was initiated by server, but cancel actually initiated by client.
This removes the old cancel method and changes the tokens mapping of progress instance so that it holds future corresponding for each tokens. The future are cancelled when a work done progress cancellation notification is received. This way, server code can either check the cancelled method of the future, as it is done in the json example:

pygls/examples/json-extension/server/server.py

Lines 208 to 211 in 130f52a

	# Check for cancellation from client
	if ls.progress.tokens[token].cancelled():
	# ... and stop the computation if client cancelled
	return

The example json extension has been updated, these changes can be tested by:

• start the json extension
• open a json file
• use start the progress command
• click on the progress bar
• click cancel button

cancel_progress.mov

This also contain some related fixes - that I can submit separately if you think it's better:

• WorkDoneProgressOptions.workDoneProgress type was incorrect (I don't know how to test, vscode does not support this at this time)
• changes to the test
• added a type annotation for better autocompletion in editor

Everything should be detailed in individual commit messages.

Fixes #233

Code review checklist (for code reviewer to complete)

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
• Title summarizes what is changing
• Commit messages are meaningful (see this for details)
• Tests have been included and/or updated, as appropriate
• [/] Docstrings have been included and/or updated, as appropriate
• [/] Standalone docs have been updated accordingly
• [/] CONTRIBUTORS.md was updated, as appropriate
• [?] Changelog has been updated, as needed (see CHANGELOG.md)

[perrinjerome]

To prevent confusion, this is about cancellation, but a completely different one from the Cancel notification for unknown message id ... issue that was discussed in #240 (comment) .

[tombh]

It looks really good to me. Though I'm not very familiar with the area. Do you think it warrants anding to the changelog? Anyway, from my perspective I'm happy to merge it. Would you like someone with more experience of the area to review as well?

[perrinjerome]

Thanks @tombh

Do you think it warrants anding to the changelog?

yes, we should probably add a line in Added section, with something like that:

Support cancel notification for work done progress tokens

Would you like someone with more experience of the area to review as well?

I believe the part that is most important to discuss is the added API, the is_cancelled and on_cancel methods from

pygls/pygls/progress.py

Lines 77 to 87 in db66295

	def is_cancelled(self, token: ProgressToken) -> bool:
	"""Check if work done progress was cancelled.
	"""
	return self.tokens.get(token) == _ProgressTokenStatus.Cancelled
	def on_cancel(self, token: ProgressToken, cancel_callback: Callable[[], None]) -> None:
	"""Register a callback to be called if the if work done progress is cancelled.
	"""
	if token in self.tokens_cancel_callbacks:
	raise ValueError(f'Cancel callback for {token} already registered.')
	self.tokens_cancel_callbacks[token] = cancel_callback

This adds two methods for more or less the same thing ... that does not smell so good. The LSP architecture made it easy to have a on_cancel callback, but having a is_cancelled sync method seemed to be often easier to use (as we can see in the json example).

Another idea I just have is that we could expose a cancelled future that would be resolved once a cancel notification from the client is received, then from user code it would be possible to use if ls.progress.cancelled.done() or ls.progress.cancelled.add_done_callback(callback), which is functionally equivalent to the current version of this code, but without reinventing the wheel.

I'm going to make this amendment and add a changelog entry first (not sure I will have the time to do this before going on vacation)

[tombh]

Ok, sounds good.

[perrinjerome]

@tombh thanks, I changed API to expose futures directly, as discussed. While doing this I realised that progress.create and progress.create_async methods were not tested, so I added test for them ( and hit same problem as #201 - for now I added a quick workaround in the test ). I also added a changelog entry.

[karthiknadig]

@perrinjerome The API looks great. Thanks for working on this.

[tombh]

Just a friendly ping to see where we are with this?

[tombh]

This looks good to merge then! Just the Pyodide tests failing for unknown reason 🤔 Even though I see you've ignored set the new tests to ignore Pyodide, hmm.

[perrinjerome]

Hi @tombh thanks for the reminder, for me everything was "good enough to merge" at the time, but there was this discussion above regarding the XXX comment.

Today I rebased and I have pushed the new commits in this branch and I believe this is OK. The rebase was straightforward, the conflicts were mostly with imports being changed and slightly different names. Also, what was problematic with the previous version of this pull request has been solved by lsprotocol and pygls 1.0:

• the workaround for the problem discussed above with the XXX comment is no longer needed, so I removed the workaround
• the previous version of this pull request contained ff3ead8 to fix a problem with the type of WorkDoneProgressOptions.workDoneProgress; this was also fixed in lsprotocol and 688b36b so I dropped this change.

[tombh]

That's great, thank you.

[perrinjerome]

Thanks !

About the failed pyiodide tests everything looks OK on CI, so in my understanding everything is OK now. I used to have an old appveyor connected to my repository and this was failing, so maybe that was the problem. I removed it and now it's green. In any case, if these changes cause issues, let me know and I'll take a look.

[tombh]

Sure, no problem.