GoogleChrome / lighthouse

Automated auditing, performance metrics, and best practices for the web.
https://developer.chrome.com/docs/lighthouse/overview/
Apache License 2.0
28.03k stars 9.32k forks source link

Temp files not cleaned up in WSL #15610

Open chrisrrowland opened 7 months ago

chrisrrowland commented 7 months ago

FAQ

URL

https://www.google.com

What happened?

In WSL I am running this code like

node LighthouseScanner.mjs https://www.google.com

After it completes there is a temp folder left behind that looks like a full windows path.

lighthouse ls -l total 20 drwx------ 3 rowc rowc 4096 Nov 14 15:36 'C:\Users\chris\AppData\Local\lighthouse.42200815'

I don't see anything indicating an error.

import lighthouse from 'lighthouse';
import * as chromeLauncher from 'chrome-launcher';

(async () => {
  const chrome = await chromeLauncher.launch({
    chromeFlags: ['--headless', '--quiet'],
    chromePath: '/usr/bin/google-chrome-stable',
  });

  if (process.argv.length < 3) {
    console.error('Please provide a URL as a command line argument.');
    process.exit(1);
  }

  const url = process.argv[2];
  const options = {
    logLevel: 'info',
    output: ['html', 'json'],
    port: chrome.port,
    tmpdir: '/tmp/',
  };
  const runnerResult = await lighthouse(url, options);

  const combinedResult = {
    json: runnerResult.lhr,
    html: runnerResult.report,
  };

  // Output the combined result as a JSON object to stdout
  console.log(JSON.stringify(combinedResult, null, 2));

  await chrome.kill();
})();

What did you expect?

No temp files left behind.

What have you tried?

I have run it in directories not named lighthouse and the file name is still the same.

How were you running Lighthouse?

node

Lighthouse Version

11.2.0

Chrome Version

Google Chrome 118.0.5993.117

Node Version

v20.9.0

OS

WSL

Relevant log output

No response

connorjclark commented 7 months ago

This is due to chrome-launcher.

  1. https://github.com/GoogleChrome/chrome-launcher/blob/main/src/utils.ts has os-specific functions for makeTmpDir. Can we just use os.tmpdir? This is...overly complicated and there's no comments justifying doing this manually. If we use os.tmpdir it wouldn't matter as much that we do not clear them on early terminations of the process.

  2. Should we do [kill()](https://github.com/GoogleChrome/chrome-launcher/blob/main/src/chrome-launcher.ts#L399C3-L399C8) in a nodejs exit handler?

chrisrrowland commented 7 months ago

This is due to chrome-launcher.

That's interesting, I opened an issue over there a couple of weeks ago, but I assumed the temp file names meant this particular bit was a lighthouse issue.

connorjclark commented 7 months ago

yeah I'm sure through a series of copy/paste we just left that in when extracting chrome-launcher out of lighthouse forever ago.

Small curiosity: this folder only has "lighthouse" in the name for win32/unix, not when under WSL, if I'm reading util.ts correctly. Since you are running on WSL, but still get this "lighthouse" named temp folder, I suppose our getPlatform() / isWSL code isn't working as we expected. I've never used WSL myself, and I believe this was 100% an external contribution, so I'll install WSL and play with it a bit to see what's going on.

@paulirish do you know why chrome-launcher does so much work to create a temp dir in a specific place, rather than just use os.tmpdir? Like, for WSL it tried to grep a user name from PATH to create a folder in that user's appdata, which just seems kinda intense. What's the importance of making these folders in a specific place? seems any ol' tmpdir given by the OS would suffice.

I'll give it a shot on my windows machine.

paulirish commented 7 months ago

@paulirish do you know why chrome-launcher does so much work to create a temp dir in a specific place,

no idea.

chrisrrowland commented 7 months ago

I suppose our getPlatform() / isWSL code isn't working as we expected.

I'm pretty sure isWsl is correctly identifying my environment as WSL but I don't think the regex is working correctly. I had thrown out the idea of a parameter to specify a temp dir but os.tmpdir seems like it makes more sense.

Thanks for your attention on the matter!

rudiedirkx commented 5 months ago

I'm getting even weirder paths after running lighthouse. ls -lAh:

drwx------   3 rudie rudie 4.0K Jan 16 09:36 '\\wsl.localhost\Ubuntu\var\www\dip\undefined\Users\undefined\AppData\Local\lighthouse.98673163'/

I'm in /var/www/dip and that's where I ran node node_modules/lighthouse/cli/index.js.

What are the 2 undefineds? With a special unicode (?) character? And a Windows username?

I have lighthouse@11.4.0 and chrome-launcher@1.1.0. It does work well, I didn't even expect that. Well done WSL and Lighthouse 👍

rudiedirkx commented 3 months ago

If I change getPlatform() to just return process.platform and never 'wsl', everything works perfectly. Maybe this is a WSL 1 vs 2 thing? Maybe wsl specific code was necessary in WSL 1, but that's what 'breaks' WSL 2, which is 'real' linux.