[WIP / Please Comment] Remove Asynchrony / Fix Races? #343#368
[WIP / Please Comment] Remove Asynchrony / Fix Races? #343#368
Conversation
|
Update: looks like I'm still experiencing races, sadly :( will have to look closer. |
|
|
||
| try { | ||
| return await f(); | ||
| return f(); |
There was a problem hiding this comment.
Thanks for looking into this! Even if it does work this is not desirable; we want to await f here, otherwise we won't catch potential errors.
There was a problem hiding this comment.
This seems very flaky unfortunately. As said in the ticket, whether a problem occurs or not is very dependent on hardware (and the rest of the system); I encounter the bug fairly frequently on my workstation after hours of coding, but not at the start of a session or on my home computer (M3 MacBook).
There was a problem hiding this comment.
Same for me with my high-end workstation, is there any memory leak or doing some operation whose complesity grows with time? For me it only happens when the workspace is large and keep open for hours.
There was a problem hiding this comment.
I expect there's a memory leak somewhere, but haven't found it. At some point I expected the problem was the history buffer growing, but even after limiting it in a local build of Dance nothing changed.
It's also been much, much worse for the past couple of months. Now both my workstation and MacBook start to struggle hard after <1 hour of editing. It seems different documents degrade at different rates, though.
Hi 71 -
I took the liberty of checking out the code and seeing if I could address this keypresses racing issue (#343). The first thing I noticed is that the very start of the keypress handling stack is asynchronous. What I've learned is that
awaiting a concrete value still yields to the scheduler. So, we should avoid asyncs or awaits in the keypress stack.As a first pass, I simply removed the await here. Because of the use of the unknown type, I can't identify which values were promises and which were concrete, but it passes the entire test suite and my initial experimentation hasn't indicated any problems yet. I thought maybe you would be able to comment on where asynchrony is actually needed.
If we do need to maintain asynchrony, it should be easy to use a conditional to either await or return; this may require restructing a little bit in the very top of the stack where we install handlers to the extension.
I'm going to try building this and installing it on my main VSCode to see if it fixes the problem and/or causes new problems, since it's extremely difficult to properly test this behavior. Can you let me know if I'm on the right track?
Thank you!