Skip to content
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

Temporary directories are not removed on VS Code exit #4142

Open
2-5 opened this issue Nov 5, 2022 · 22 comments
Open

Temporary directories are not removed on VS Code exit #4142

2-5 opened this issue Nov 5, 2022 · 22 comments
Labels
addressed in next version Issue is fixed and will appear in next published version bug Something isn't working

Comments

@2-5
Copy link

2-5 commented Nov 5, 2022

Describe the bug
Pyright doesn't clean up the temporary directories it creates.

To Reproduce
Edit Python code in VS Code while using the Pylance extension, notice how Pyright creates temporary directories which are not removed on VS Code exit.

I have discovered a thousand (almost all empty) Pyright directories in my TEMP:

pyright-5808-bG8MfxYSKNtF
pyright-5808-OIkd54bPdJ2d
pyright-5808-rdZaBCaNhflA
pyright-5808-v5rwRxEF8MuH
pyright-7658-0xvc2QBt5k9N
pyright-7658-d6Y4QYAdwjj3
pyright-7658-q41TUWxoh6xR
...

Expected behavior
Pyright removes temporary directories on VS Code exit.

VS Code extension or command-line
VS Code with Pylance v2022.11.10

Additional context
Temporary directories are created here, but don't seem to be removed anywhere:

tmpdir() {
if (!this._tmpdir) {
const dir = tmp.dirSync({ prefix: 'pyright' });
this._tmpdir = dir.name;
}
return this._tmpdir;
}

@erictraut erictraut added the needs investigation Requires additional investigation to determine course of action label Nov 5, 2022
erictraut pushed a commit that referenced this issue Nov 5, 2022
…etermining whether the file system is case sensitive. This addresses #4142.
@erictraut erictraut added bug Something isn't working addressed in next version Issue is fixed and will appear in next published version and removed needs investigation Requires additional investigation to determine course of action labels Nov 5, 2022
@erictraut
Copy link
Collaborator

This will be addressed in the next release.

@2-5
Copy link
Author

2-5 commented Nov 5, 2022

Thanks for the quick fix!

Out of curiosity I looked at the resolve commit, and I noticed what seems like a bug to me. I could be totally wrong since I don't know the code base at all.

The temporary directory which was just created is immediately removed:

if (tempDirPath) {
try {
// Remove temp directory.
fs.rmdirSync(tempDirPath);
} catch (e: any) {
// Ignore exception.
}
}

But at the same time, tmpdir path is cached inside RealFileSystem:

tmpdir() {
if (!this._tmpdir) {
const dir = tmp.dirSync({ prefix: 'pyright' });
this._tmpdir = dir.name;
}
return this._tmpdir;
}

Doesn't that mean that the next caller of RealFileSystem.tmpdir() will receive a temporary directory path which doesn't exist anymore? Unless the RealFileSystem instance is discarded immediately.

erictraut pushed a commit that referenced this issue Nov 5, 2022
…s when determining whether the file system is case sensitive. This addresses #4142."

This reverts commit c5d1cfc.
@erictraut
Copy link
Collaborator

Ah, that's a good point. I didn't realize that pyright's own tmpdir method was creating and caching this subdirectory.

This is the only place we use tmpdir in pyright, so deleting it immediately happens to work, but if we ever use tmpdir for something else in the future, it will create a problem.

I found a much better solution.

@2-5
Copy link
Author

2-5 commented Nov 5, 2022

This is the only place we use tmpdir in pyright

I think there is another place, this code seems to create empty files, but I also have these orphaned files on my system:

TEMP\pyright-17048-FK3xbAzNz3Uj
  builtins-17048-7imD9MLcmYyW-.py
  builtins-17048-rEk0QRGdGHR3-.py
  ntpath-17048-6l9l1Zbtzd52-.py
  posixpath-17048-Bbsm5zQ8O6Uc-.py

They seem to be auto-generated typing stubs. builtins-17048-7imD9MLcmYyW-.py starts like this:

# Python: 3.9.13 (tags/v3.9.13:6de2ca5, May 17 2022, 16:36:42) [MSC v.1929 64 bit (AMD64)]
# Library: builtins, version: unspecified
# Module: builtins, version: unspecified

"Built-in functions, exceptions, and other objects.\n\nNoteworthy: ..."

import typing
class ellipsis(object):
    def __getattribute__(self, name) -> typing.Any:
        'Return getattr(self, name).'
        ...

    def __init__(self, *args, **kwargs) -> None:
        ...

@erictraut
Copy link
Collaborator

This is included in pyright 1.1.279, which I just published. It will also be included in a future version of pylance.

@2-5
Copy link
Author

2-5 commented Nov 17, 2022

@erictraut Today my Pylance updated to version v2022.11.30 which includes Pyright 1.1.279 with the fix, but it doesn't seem to work. I've deleted all the pyright temporary directories, started VS Code, closed it, and the pyright temporary directories are not removed. I've confirmed this 3 times.

[Info  - 14:03:01] (7684) Pylance language server 2022.11.30 (pyright 04a10583) starting

@erictraut erictraut reopened this Nov 17, 2022
@erictraut erictraut removed the addressed in next version Issue is fixed and will appear in next published version label Nov 17, 2022
@erictraut
Copy link
Collaborator

I confirmed that the temp directory created in pyright's main process is correctly cleaned up. However, the temp directory created in the background (worker) process is not. I didn't notice this previously because the debug version of pyright disables the background (worker) process to aid in debugging.

My working theory is that this is a bug in node's handling of setGracefulCleanup for workers. We'll need to investigate further.

@rchiodo
Copy link
Collaborator

rchiodo commented Nov 17, 2022

My guess is that the worker threads just die, they don't get a chance to run any cleanup. We might have to post a message to the main thread in order to generate tmp folders/files.

Looks like tmp could fix this if it knew it was in a worker by listening to worker.exit and not just process.exit
https://nodejs.org/api/worker_threads.html#event-exit

This code here in tmp:
https://github.com/raszi/node-tmp/blob/da3bc1ac9a7880def4a3129c4f8bfe96a63bf99c/lib/tmp.js#L655

That code will only run on the main thread.

@rchiodo
Copy link
Collaborator

rchiodo commented Nov 17, 2022

I bet you could do something like this in tmp.js

if (isMainThread) {
  process.on(EXIT, _garbageCollector)
} else {
  worker.on(EXIT, _garbageCollector)
}

@debonte debonte added the addressed in next version Issue is fixed and will appear in next published version label Nov 19, 2022
@erictraut
Copy link
Collaborator

This is addressed in pyright 1.1.281, which I just published. It will also be included in a future release of pylance.

@noootch
Copy link

noootch commented Jan 17, 2025

Hello,
I found this issue when I was trying to find out why my temp folder had 60gb which wouldn't be removed. I am using the latest versions of vscode and its plugins along with Python 3.11.9.
The issue described here still exists. When cleaning up the temp folder I removed a few thousand pyright-XYZ folders.

@erictraut
Copy link
Collaborator

@noootch, which version of pyright are you using?

@noootch
Copy link

noootch commented Jan 17, 2025

@noootch, which version of pyright are you using?

I'm not using it directly but through Pylance 2024.12.1. They use Pyright 1.1.389.

@2-5
Copy link
Author

2-5 commented Jan 17, 2025

I can confirm it too:
Image

@erictraut
Copy link
Collaborator

Thanks, I'm able to repro as well. Looks like something regressed here. Reopening issue.

@erictraut erictraut reopened this Jan 17, 2025
@erictraut erictraut removed the addressed in next version Issue is fixed and will appear in next published version label Jan 17, 2025
@heejaechang heejaechang self-assigned this Jan 17, 2025
@heejaechang
Copy link
Collaborator

let me take a look.

@heejaechang
Copy link
Collaborator

the issue was the library we use for temp folder. the auto deletion only works for the main process. any new temp folder created by worker or forked process; auto deletion didn't work. fixed by making main process own the temp folder and all other one to use temp folder created by the main process.

@erictraut
Copy link
Collaborator

@heejaechang, it apparently regressed. I'm seeing thousands of temp folders remaining on my machine. I guess some of these could be because I'm killing the pyright process when debugging issues, but I don't think that explains all of them. I'll need to investigate and confirm, but I think there's something broken here.

@erictraut
Copy link
Collaborator

@heejaechang, when you say "fixed by making main process", do you mean that you previously introduced a fix? Or you are working on a new fix that you haven't checked in yet? I think your analysis and conclusion is correct — that this is caused by temp folders created by the worker or forked process. The temp folders created by the main process do appear to get cleaned up correctly unless pyright crashes or is abnormally terminated (e.g. by an attached debugger).

@heejaechang
Copy link
Collaborator

it apparently regressed. I'm seeing thousands of temp folders remaining on my machine. I guess some of these could be because I'm killing the pyright process when debugging issues, but I don't think that explains all of them. I'll need to investigate and confirm, but I think there's something broken here.

yes, I just pushed fix to pyright, can you try again and check whether it is fixed? basically there are 2 fixes.

  1. made sure unit tests delete temp folders they created (basically making sure all tests to dispose temporary file service if it is used)
  2. made sure all child process or worker threads to use temp folder owned by main worker thread rather than creating their own.

@heejaechang
Copy link
Collaborator

when you say "fixed by making main process", do you mean that you previously introduced a fix? Or you are working on a new fix that you haven't checked in yet? I think your analysis and conclusion is correct — that this is caused by temp folders created by the worker or forked process. The temp folders created by the main process do appear to get cleaned up correctly unless pyright crashes or is abnormally terminated (e.g. by an attached debugger

sorry, missed this message. I just pushed the fix. I had to make fix in pylance and push since pylance had the same issue and I had to fix all together.

@erictraut
Copy link
Collaborator

I checked out your fix and confirmed that it's working correctly on my machine. Thanks!

@erictraut erictraut added the addressed in next version Issue is fixed and will appear in next published version label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants