Repository navigation
run handlers after responding instead of from set_timeout_async - #2927
Conversation
| def request_handler( | ||
| method: str | ||
| ) -> Callable[[Callable[[Any, P], Promise[R]]], Callable[[Any, P, int], Promise[Response[R]]]]: | ||
| ) -> Callable[[Callable[[Any, P], RequestHandlerResponse]], Callable[[Any, P, int], Promise[Response[R]]]]: |
There was a problem hiding this comment.
Do we really need this for plugins too? This seems to be quite niche problem that only occurs for the "refresh" requests, because we want to send the response for those first before doing further actions. I wouldn't expect the same to be so relevant to custom LSP requests, so I would assume it to be sufficient for plugins to do the work first, before sending the response, or alternatively live with set_timeout / set_timeout_async.
This possibility to return a tuple with additional callback is currently not mentioned in the docstring.
There was a problem hiding this comment.
I agree and that's why I have not documented it. I would consider this an internal feature even if it leaks to the public API.
There was a problem hiding this comment.
I wonder wouldn't it be better then to allow this only for the internal code, perhaps by using a different decorator variant that is not exposed in the public API?
But maybe it's not so important if we prefer to avoid the code duplication that might be needed for that?
There was a problem hiding this comment.
Added complexity and maintenance wouldn't be worth it IMO.
It's not documented so it's not clear what it does without reading internal code so that should keep people away from using it.
And if someone uses it, nothing bad will happen anyway.
|
If I may be honest here: we are going to have to solve this problem again with the asyncio PR. And the solutions here donβt translate cleanly to asyncio. Soβ¦ maybe pause this? |
No, it should translate just fine, I think this should work: 7276e53 |
rchl
changed the title
Alright, I'll trust you then. π |
|
I think I've more or less gotten two OKs, even if not explicit, so will merge and update the asyncio task after. |
I thought hard how to avoid set_timeout_async but still run handler after responding and this is the solution.
Request handler can return a
tuple(Promise, callback)instead ofPromiseto runcallbackright after responding, without posting a new task withset_timeout_async.Fixes #2926