-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix hanging MessagesPoll.Poll
#4325
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
Changes from all commits
243f533
9697813
ceb61d4
c09e0f3
8043981
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| from ycm.client.base_request import BaseRequest, BuildRequestData | ||
| from ycm.vimsupport import PostVimMessage | ||
|
|
||
| import json | ||
| import logging | ||
|
|
||
| _logger = logging.getLogger( __name__ ) | ||
|
|
@@ -56,8 +57,32 @@ def Poll( self, diagnostics_handler ): | |
| # Nothing yet... | ||
| return True | ||
|
|
||
| response = self.HandleFuture( self._response_future, | ||
| display_message = False ) | ||
| # Avoid HandleFuture() to prevent blocking in timer callbacks. | ||
| # HandleFuture() does: | ||
| # 1. Complex exception handling (UnknownExtraConf, DisplayServerException) | ||
| # 2. User dialogs that can block waiting for input | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it doesn't, at least, not in any relevant codepath. |
||
| # 3. Vim UI updates during callback execution | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So does this code. I mean, that's its literal job : HandlePollResponse will do UI updates, that's what it's for. |
||
| # By extracting the response directly with minimal error handling, we avoid | ||
| # blocking vim's main thread. Note that response.read() is still technically | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. citation needed? If response.done() is True then HandleFuture (which just calls JsonFromFuture) will just do the exact thing this code does, except this code skips HMAC validation, which is absolute Nope. Skipping HMAC validation is a security issue and we can't accept it. I'm honestly not convinced by this change, and the explanation seems ... synthetic to me. I'm willing to believe there is indeed an issue when there are very big notification messages (such as huge diagnostics) but I'd like to have a more convincing explanation and maybe the flame graph, profile, or whatever was mentioned in the description.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the detailed review. I'm sorry you're upset with a generated answer. I didn't test the fix well enough. Most likely, I concluded it was fixed before the entire dataset was indexed by Here's the original flamegraph + perf.script data I used for it. The second flamegraph looks similar, since I waited until the data was loaded. For the one that showed an improvement, I only have a flamegraph; the perf data has been overwritten. I'll dig a bit deeper into it, but it looks like there's no easy solution. Thank you for your time, and happy New Year 2026! |
||
| # blocking, but: | ||
| # - The future is already done(), data received from localhost ycmd server | ||
| # - Network I/O is complete, read() just copies from buffer to memory | ||
| # - No user interaction or complex processing | ||
| # The real performance issue was HandleFuture's heavy exception handling, | ||
| # not the I/O itself. | ||
| try: | ||
| response = self._response_future.result( timeout = 0 ) | ||
| response_text = response.read() | ||
| response.close() | ||
| if response_text: | ||
| response = json.loads( response_text ) | ||
| else: | ||
| response = None | ||
| except Exception: | ||
| _logger.exception( 'Error while handling server response in Poll' ) | ||
| # Server returned an exception. | ||
| return False | ||
|
|
||
| if response is None: | ||
| # Server returned an exception. | ||
| return False | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so? why is this a problem in this case? do we have a trivial/minimal repro case?