Uh oh!
There was an error while loading. Please reload this page.
add async interface - #12
Conversation
Uh oh!
There was an error while loading. Please reload this page.
| self._executor_service = futures.ThreadPoolExecutor(max_workers=max_workers) | ||
| self._cancelledRequests = set() | ||
| def init_async(self): |
There was a problem hiding this comment.
Shouldn't we have a way to set a asyncio event loop executor by parameter? https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.set_event_loop
There was a problem hiding this comment.
This init_async function was more a relict of a previous attempt... I think it is save to initialize the queue in the default initializer...
Regarding set_event_loop I don't how I achieve something similar to davschul/python-lsp-server@215f79d in a more ellegant way, And I'm open to suggestions :)
There was a problem hiding this comment.
No worries, this can be looked in a future iteration.
Uh oh!
There was an error while loading. Please reload this page.
| async def consume_task(self): | ||
| log.warning("starting task") | ||
| loop = asyncio.get_running_loop() | ||
| while True: |
There was a problem hiding this comment.
Is this infinite loop being exited at any point? i.e., what happens at teardown time
There was a problem hiding this comment.
I guess the loop is never cleanly exited, let me check what python has to offer to stop this loop here.
andfoy
commented
Feb 8, 2022
@davschul, thanks for the changes, we would appreciate if you added some tests for the async interface |
davschul
commented
Feb 16, 2022
The last changeset addresses the shutdown, I'll have another look for the tests next |
davschul
commented
Feb 16, 2022
Took a first stab at adding tests |
| def init_async(self): | ||
| self._messageQueue = asyncio.Queue() | ||
| self._consume_task = asyncio.create_task(self.consume_task()) |
There was a problem hiding this comment.
Will this task run on the executor?
There was a problem hiding this comment.
This task will not run in an executor from https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.run_in_executor, but needs to be started after the event loop was started. I pushed an example implementation to davschul/python-lsp-server@9f0f03d
There was a problem hiding this comment.
The infinite loop has potential to block the main event loop, which could lead to performance degradation/unexpected blockage. I think in this case it would be better to run this on a separate event loop
There was a problem hiding this comment.
The asyncio queues used to send the messages cannot be used from different event loops. What might cause the block here in your opinion?
There was a problem hiding this comment.
Sorry for the long break, I was side tracked a bit in the python lsp support in Qt Creator and some urgent bugs before our release. I added a restriction to the loop, but I guess that not really what you've expected right?
andfoy
commented
Jun 8, 2022
Sorry for forgetting about this one! I think it looks better than a |
andfoy
commented
Jun 8, 2022
I don't know why the tests are not running |
ccordoba12
commented
Jun 8, 2022
Closing and reopening to see if they run. |
bicobus
commented
Nov 3, 2022
Sorry to bother @andfoy and @ccordoba12 ; has this PR been forgotten? It seems somewhat helpful. |
ccordoba12
commented
Nov 3, 2022
No, it hasn't. The thing is we also need to make |
This is a very early version of async message reading with a request for comments whether this in general would be acceptable and what I need to do to make sure this doesn't break anything.