[BUG] Lock not released in case of ECONNRESET when installing

[alexdima]

Context:

• Playwright Version: All
• Operating System: All
• Node.js version: 14.x
• Browser: All
• Extra: GitHub Action / Azure Pipelines

Background

We use caching of node_modules foders to speed up our CI build times and we execute yarn with PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD, so oftentimes node_modules are restored but the playwright downloads are not restored anyways, so we then run the install script by ourselves.

So on our build machines we invoke node node_modules/playwright/install.js and we see every now and again that builds fail due to ECONNRESET errors when trying to install playwright. e.g. https://github.com/microsoft/vscode/runs/1747455678#step:12:130

That is to be expected since internet might always be flaky, and the normal path forward is to just retry downloading.

Problem

We've added some code to retry installing on our side e.g.

export async function retry<T>(fn: () => Promise<T>): Promise<T> {
	for (let run = 1; run <= 10; run++) {
		try {
			return await fn();
		} catch (err) {
			if (!/ECONNRESET/.test(err.message)) {
				throw err;
			}

			const millis = (Math.random() * 200) + (50 * Math.pow(1.5, run));
			console.log(`Failed with ECONNRESET, retrying in ${millis}ms...`);

			// maximum delay is 10th retry: ~3 seconds
			await new Promise(c => setTimeout(c, millis));
		}
	}

	throw new Error('Retried too many times');
}

const { installBrowsersWithProgressBar } = require('playwright/lib/install/installer');
const playwrightPath = path.dirname(require.resolve('playwright'));
await retry(() => installBrowsersWithProgressBar(playwrightPath));

So basically we invoke installBrowsersWithProgressBar again and again until it stops throwing ECONNRESET but at most 10 times.

The problem is that we now get builds failing with Lock file is already being held errors e.g. https://github.com/microsoft/vscode/runs/1748968312#step:10:264

From a quick look there is some locking done in that function and the locks are not released in the error path. IMHO the problem might be in installBrowsersWithProgressBar which perhaps should wrap await releaseLock(); in a finally block.

If you have a better way to install playwright which is resilient to intermittent network errors, I would be happy to try that out instead.

[aslushnikov]

@alexdima awesome investigation! I've adopted your snippet code into Playwright to run download re-tries.

[aslushnikov]

@alexdima so the fix has landed and is available at the playwright@next channel. We'll be happy if you can test this out and see if it makes things better for you. If it doesn't, please feel free to open a new issue to bring our attention to it! (we often miss notifications to the closed issues but are very carefully triaging all the incoming bugs).