Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add event loop #2576

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add event loop #2576

wants to merge 4 commits into from

Conversation

predragnikolic
Copy link
Member

@predragnikolic predragnikolic commented Dec 8, 2024

This PR:

  • adds a run_future function .
  • gives an example plugin on how to use async/await with ST (that plugin will be deleted). The view_async.py content could stay, but the file would need to be renamed and put in a proper place. (please say if it should stay).

This PR will not do a huge refactor of the existing code,
instead it just add the first building block to use asyncio and futures. Nothing more, nothing less.

Why?
Currently we have Promises thanks to @rchl.
Promises allowed us to have Futures before we could use Futures. Now that we can use Futures, it will allow us to use the following asyncio apis:

  • asyncio.gather(list_of_futures) await a list of futures
  • asyncio.wait_for(future, 3) # run future, but cancel the future it runs for more then 3 seconds
  • future.cancel cancel a future.

Here is an example of what could be build by using, gather, wait_for, and cancel:

class lsp:
    @staticmethod
    async def definitions(view: sublime.View, point: int) -> list[tuple[SourceName, Definition | list[LocationLink] | None]]:
        # STEP 1:
        providers = [provider for provider in definition_providers]
        for provider in providers:
            await provider.cancel() # cancel already running futures

        # STEP 2 define return value
        results: list[tuple[SourceName, Definition | list[LocationLink] | None]] = []

        # STEP 3:
        async def handle(provider: DefinitionProvider):
            try:
                # cancel the future if the plugin doesn't provide results in 2 seconds 
                result = await asyncio.wait_for(provider.provide_definition(view, point), 2)
            except Exception as e:
                print(f'Error happened in provider {provider.name}', e)
                return (provider.name, None)
            return (provider.name, result)

        try:
            results = await asyncio.gather(
                *[handle(provider) for provider in providers],
            )
        except Exception as e:
            print('LSP (DefinitionError):', e)
        return results

The one who originally wrote the code is Raoul Wols
Co-authored-by: Raoul Wols <[email protected]>

class EventLoopListener(sublime_plugin.EventListener):
def on_exit(self) -> None:
shutdown_event_loop()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the event loop starts, when the LSP module is loaded,
and the loop ends when ST exits.

@@ -0,0 +1,27 @@
import sublime_plugin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file will be deleted.

Comment on lines 21 to 27
view = await open_view(readme_file, w)
view.run_command("append", {
'characters': "LspExampleAsyncCommand added this" + '\n\n',
})
await save_view(view)
# the view is saved at this point and safe to be closed
view.close()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These few lines illustrate one of the benefits.
On line 22, the view is guaranteed to be loaded.
After line 25, the view is guaranteed to be saved.

from .view_async import open_view, save_view
from pathlib import Path

class LspExampleAsyncCommand(sublime_plugin.TextCommand):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to try this command, here is a keybinding {"keys": ["primary+i"], "command": "lsp_example_async" } or view.run_command('lsp_example_async') if you want to run it from the ST console.

@predragnikolic
Copy link
Member Author

I have removed the unnecessary files, if you want to try the example, checkout the commit bd7cdae

@rchl
Copy link
Member

rchl commented Dec 10, 2024

We run our logic either on the main or ST's async thread. When running a feature then on what thread those run technically?

I'm thinking that it could open up for some interesting race conditions if we mix ST async's thread and features...

@predragnikolic
Copy link
Member Author

I don't have the necessary knowledge to give the right answer, but I will give my understanding after spending time trying out the asyncio solution that rwols wrote on a different ST plugin.

The event loop is started on a separate thread https://github.com/sublimelsp/LSP/pull/2576/files#diff-0f06b8214f4534bf9cfc50065f635b6bc543bc54f72946858a476319da98c4bbR29 .
Any future run with run_future will be run on that background thread. Not the ST main thread, not the ST async thread.

I did run into issues where promises got garbage collected before the finished.
I have patched that by storing the futures in this __active_tasks : set[asyncio.Task] set, to not let the GC collect them. (I see that as a proper fix/patch, but others might not agree)
https://github.com/sublimelsp/LSP/pull/2576/files#diff-0f06b8214f4534bf9cfc50065f635b6bc543bc54f72946858a476319da98c4bbR9

There will be issues, I don't doubt that. But as with any issue, a solution might appear over time, or the issue will stay.
I still thing that going the asyncio route can benefit LSP. Writing code with asyncio was easier, and the reading the code as well. There might be unforeseen issues that I cannot predict, or fix.

Here are some related/useful comments by others. sublimehq/sublime_text#3129:

  • One reasonable takeaway from that discussion is that while the proposed asyncio event loop is pretty tidy, it will still be running all tasks in a background thread, so there will still be thread safety issues between blocking calls and async calls.
  • While this is already an issue with the async thread for *_async hooks, it's true that a part of async event loops is to prevent race conditions and running the loop on a separate thread entirely defeats that purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants