Use asyncio#2880
Conversation
Add a test that checks that "tcp server mode" works. Server meaning that this plugin acts as the TCP server and the langserver connects as TCP client.
Add a test that checks that "tcp server mode" works. Server meaning that this plugin acts as the TCP server and the langserver connects as TCP client.
Conflicts: tests/server.py
✅ Deploy Preview for sublime-lsp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment was marked as resolved.
This comment was marked as resolved.
|
It's okay if #2739 is merged before this. I'll take care of the merge conflicts. |
Also write the rest of the requests in terms of Session.request
|
Would be useful to split it into smaller chunks, if possible. For example it would likely be possible to make Lots of assumptions on my side but that's what I feel. I was actually looking into that before as I wanted to move start_async into dedicated thread so that it doesn't black other plugins (kinda opposite goal of yours but also kinda similar as I guess with asyncio it will also run on dedicated thread). See #2863 It will be hard to review it properly with a big dump of code that refactors most of the code base. |
- Add sublime.set_timeout executor wrapper - Make all request handlers `async` - Define a CancellableInflightStreamingRequest class that enables `async for` syntax - Start inheriting DocumentSyncListener from sublime_aio.ViewEventListener (This one doesn't work yet) The state is fairly broken at this point.
Takes a list of exceptions and prints all of them. Also expand the `trace` function so it can print stack traces and optionally some values.
THis exception can occur when "violently" opening and closing tabs.
…cancel_all_tasks aclosing: is to be used in an `async with` context for safely closing asynchronous generators (like the CancellableInflightStreamingRequest). gather_and_flatten_exceptions: when using asyncio.gather with the return_exceptions=True keyword argument, and when all coroutines in the asyncio.gather already return lists of exceptions, then flatten the lists and filter out exceptions that are not Exception (but do inherit from BaseException). TaskContainer.cancel_all_tasks: using the __del__ magic method for the TaskContainer is unreliable as it may be destroyed from any thread (and in fact, for things like SessionBuffer, these objects do get created and destroyed in random threads that we don't even control. Just put a trace(print_exceptions=True) in the __del__ of SessionBuffer). So introduce an explicit TaskContainer.cancel_all_tasks async method that cancels all tasks (and awaits their cancellation).
An asyncio.Lock class has the restriction that it should only be constructed in the asyncio thread. This is a restriction up to python 3.10. From python 3.10 onwards you can construct these objects from any thread.
…est, and proper shutdown mechanism
Fixes for python 3.8 runtime: can't construct asyncio.Lock on any other
thread than the asyncio thread, so just construct it just-in-time in the
WindowManager.
asyncio Locks are interesting in that you don't actually need to care
about the critical section just before the lock. Only at `await`
("suspension") points does the locking mechanism matter.
CancellableInflightStreamingRequest: uses an asyncio.Queue for properly
storing partial results if there's no awaiter yet.
Shutting things down: cancelling all tasks explicitly, and tweaks to
the Session automatically shutting down when there are no more views
attached to it.
| async def cancel_all_tasks(self) -> list[Exception]: | ||
| return [x for x in await asyncio.gather(*self._tasks, return_exceptions=True) if isinstance(x, Exception)] | ||
|
|
||
| def create_task(self, coro: Coroutine[object, object, object], /, **kwargs: Any) -> asyncio.Task: |
There was a problem hiding this comment.
Are you on purpose using / instead of (stricter) * marker?
In this case the upstream API also uses * so it would make sense to follow but I think that * generally is nicer due to being stricter.
Also instead of kwargs: Any I would manually just repeat the upstream arguments. In this case it's just a single name anyway.
| @override | ||
| def on_before_tasks(self) -> None: | ||
| sublime.set_timeout_async(self._trigger_on_pre_save_async) | ||
| call_soon_threadsafe(self._trigger_on_pre_save_async) |
There was a problem hiding this comment.
(I'm back at naming again :))
How about we call this one call_soon_on_async_thread?
call_soon_threadsafe really doesn't tell me much about:
- which thread it's gonna be called on
- what is really thread safe about it
There was a problem hiding this comment.
And as for run_coroutine_threadsafe, how about run_on_asyncio_thread? The argument type should enforce that it's used only with coroutines so IMO that doesn't need to be specified. And threadsafe is kinda useless detail IMO, especially since there is no non-threadsafe variant exposed.
There was a problem hiding this comment.
For context: I named these functions because they are so named in the asyncio module: AbstractEventLoop.call_soon_threadsafe and asyncio.run_coroutine_threadsafe.
* I missed calling Session._handle_plugin_on_pre_send_response_async in session.on_payload * Re-introduce a (deprecated) Session.execute_command_async * Invoke response handler / notification handler right away
* Session.__getattr__ exists, so type checkers do not flag incorrect method calls. (found thanks to diagnostics tests). * When you put a Coroutine object into a Promise.then, then... nothing happens. This caused on-save tasks to be broken (found thanks to code action tests). * Fix a missing call to run_coroutine_threadsafe in completion.py causing the shift-selected behavior to be broken (found thanks to completion tests). I'm having a hard time getting the FileWatcher tests working locally.
* Fix a bug in DocumentSyncListener.on_load * Debugging code action tests
| if plugin.on_open_uri_async(uri, callback): | ||
| return promise.then(lambda tup: self.open_scratch_buffer(*tup, flags, group)) \ | ||
| .then(lambda view: self._on_sheet_opened(view.sheet(), uri, r)) | ||
| title, content, syntax = await promise | ||
| view = await self.open_scratch_buffer(title, content, syntax, flags, group) | ||
| self._on_sheet_opened(view.sheet(), uri, r) | ||
| # resolve unused promise | ||
| resolve(('', '', '')) | ||
| return None | ||
| return False |
There was a problem hiding this comment.
The if plugin... block should likely have its own return.
Also can you change the return type to sublime.View | False | None since we don't expect True?
| if isinstance(self._plugin, LspPlugin): | ||
| scheme, _ = parse_uri(uri) | ||
| if handler := self._plugin.get_uri_handler(scheme): | ||
| return handler(uri, flags).then(lambda sheet: self._on_sheet_opened(sheet, uri, r)) | ||
| sheet = await handler(uri, flags) | ||
| await loop.run_in_executor(executor_main, self._on_sheet_opened, sheet, uri, r) |
There was a problem hiding this comment.
Most likely missing return here also
There was a problem hiding this comment.
I guess it also doesn’t need the run_in_executor, unless it does, in which case the other call site is wrong… but the current code also doesn’t make sure it runs on the main thread so I’ll remove this run_in_executor wrap.
5 participants
This is for now a draft PR where I'm trying to introduce asyncio and replace callback-based code with
asyncfunctions. I'll keep a todo list here for the things that I believe need to be done before marking this work as ready for review. You are free to comment, make suggestions, and dispute, during the draft of course. Lots of functionality does not work at all at the moment of opening this work as draft.The main driver for doing this is to decrease the thread usage of this plugin from O(n) to O(1) threads, where
nis the number of language servers running. The secondary driver is syntax sugar.WindowManager.startasyncWindowManager.startworkWindowManager, replace by async lock@request_handlerwork with bothasyncand-> Promise[...]variantssublime.set_timeout_asyncLSP.plugin.core.aio.call_soon_threadsafedef f() -> Promise[T]: ...async def f() -> T: ...Promise.then(lambda x: ...)x = await f()session.send_request_async(R(), lambda x: ...)x = await session.request(R())try ... except ResponseException:blockasync for partial_result in session.stream(R()):(caveat: only works forlist[...]-style responses)LSP.plugin.core.aio.run_coroutine_threadsafe(f())PromiseobjectsPromise.thenawait promisePromise.allasyncio.gathersublime.set_timeout_async(f, timeout_ms=1000)await asyncio.sleep(1)threading.Lock, or write very complicated queueing logicasyncio.Lockasyncfunction in aPromisePromise.wrap_taskfcallsgggfasync def f(): await g()async def f(): g()f, guaranteed called from asyncio threadaio.TaskContainer.create_task(g())def f(): g()f, any threaddef f(): aio.run_coroutine_threadsafe(g()), or useaio.TaskContainer.create_task_threadsafe(g())def f(): g()The Plan
Make "most" code run on the sublime_aio thread
Most code is doing bookkeeping. This type of code used to run on the Sublime "async" thread. It should run on the asyncio loop thread.
Previously, the code attempted to make most code run on the ST async thread. We never really enforced this. We tried to make it clear that a function/method should be running on the ST async thread by suffixing it with
_async.If you have an
async defcoroutine function, then such a coroutine function is forced to run on the asyncio loop thread. So enforcement becomes automatic.Keep
_asyncsuffixes, assume they run on the asyncio threadWhen a method or function has the suffix
_asyncin its name, we tried to ensure these functions run on the ST async thread. These can now be assumed to be running on the asyncio thread.Make compute-intensive function run on the Sublime "async" thread
The only compute-intensive code we deal with are parsing and emitting JSON. Only the JSON parser/emitter should run on the ST async thread.
Bridging code for existing LSP-* plugins
We made sure that all AbstractPlugin and LspPlugin related (class)methods ran on the ST async thread. I want to now make sure all these (class)methods run on the sublime_aio thread with this pull request.
Certain methods may also be marked
asyncfor LspPlugin, most notablyon_pre_startand perhapson_initialize.The
Promiseobject can be awaited, so older AbstractPlugin/LspPlugin-related functionality returning promises from request handlers should work (although currently untested).TODO: work this out futher (there's a
Promise.__await__defined, so at least we can await promises)