-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
Labels service leaks listeners in emmet integration tests #119968
Comments
I can reproduce and this seems to originate from I noticed that the emmet tests did not properly close editors on teardown and pushed b19f50e as a fix. Anyhow, should revisit the |
The |
I also played around with some ref counting of @jrieken should we maybe not print any exception if the most recent stack is |
idk - I think there is still value. Even tho, there are many, many individual listener we should take it as sign of a problem. In this case we should ask ourself if we really want hundreds or thousands of editors open at the same time |
@jrieken they are opened but also closed, at least visually I see just always 1 editor open. There are hundreds of editors opened and closed in a very short timeframe, which probably causes breadcrumbs to update a ton. At least for tabs they always share the same resource label, so that might explain why there is no warning for tabs. Agree it would be interesting if some cleanup is missing in the test run at least. |
This is very very bizarre. Commenting out just this line makes the exception go away: vscode/src/vs/workbench/browser/labels.ts Line 135 in a54d8b2
In other words, I verified that the @jrieken I am clueless why this one listener would result in the error, it looks like a very normal vscode/src/vs/workbench/browser/labels.ts Line 138 in a54d8b2
|
Hm, maybe this is a red herring and this is just the last listener that is causing the error to print but otherwise being unrelated... |
Wow, an actual leak. I didn't know that
|
@alexdima I think the concept of It seems to me that we now often have code like this: vscode/src/vs/workbench/services/preferences/browser/preferencesService.ts Lines 122 to 123 in 5f17dc2
Another example in notebook land: vscode/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts Lines 550 to 555 in 06a451d
Where we first create the Should the calls that create the
|
@bpasero I have pushed a fix for the Even with this fix, when I execute the emmet tests I still see the leakage warning initially reported. So I don't believe that was the root cause here. I am leaving the issue open for you to decide what to do:
|
@alexdima I think there is still a leak, trying to find it I added a trace to the first listener that gets added to
So it seems that
Can you advise? |
@alexdima nevermind, I figured out the root cause: we use this method to open a random file for integration tests:
Which I think is fair, given the API tests only have access to our API. However, this ends up calling I think @jrieken has reasons for this, I recall there is a timeout when we GC text file model references with the rationale that we are not giving extensions anyway to dispose those models explicitly. I pushed a change to increase the threshold for leaking in this one listener but it feels a bit like a workaround / bandaid:
Up to you and maybe Jo to decide if a different fix is needed, but I think this is just how our API works today and the integration tests manage to hit this warning because they open >175 editors in a very short period of time. |
Yes - there is a timeout and total size limit |
While checking #116775, I noticed another leak which looks like it's coming from here:
vscode/src/vs/workbench/browser/labels.ts
Line 135 in 7208128
The text was updated successfully, but these errors were encountered: