fix: retry on EMFILE always, lint sync FS calls #22175

flotwig · 2022-06-07T21:13:09Z

Closes EMFILE eventually crashes cypress open on Linux #22023
Addresses parts of Warn on all runtime fs.sync* calls that need to be converted to be async and convert to async #22028

User facing changelog

Fixed an issue where Cypress would crash with an EMFILE error if the process ran into the file descriptor limit. Cypress will now retry file reads using graceful-fs.

Additional details

Applies graceful-fs to fs on startup+nextTick to block EMFILE errors - previously, only fs-extra had graceful-fs protections
~~Adds warnings for sync FS method usage in the server when not in production~~
- I moved this to another PR: chore: warn on sync FS method use in development #22176 Since it will require more work to update sync node_modules/remaining existing sync call sites so we aren't flooded by warnings.
Adds lint rules for sync FS method usage with ignores for scripts, tests, and non-server code
- The no-restricted-syntax rule is crazy versatile, you can write arbitrary AST queries to lint using esquery syntax.
Updates trivial-to-fix places where we use sync FS methods to use async FS methods
Adds eslint-ignores in areas where it doesn't matter (debug code, etc...)
Adds TODOs and eslint-ignores where we need to do further work to remove sync FS calls

Steps to test

How has the user experience changed?

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?

cypress-bot · 2022-06-07T21:13:12Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-06-07T22:09:43Z

Test summary

4874 • 0 • 61 • 0 • 1

Run details


Project	cypress
Status	Passed
Commit	`9321032`
Started	Jun 16, 2022 1:26 AM
Ended	Jun 16, 2022 1:40 AM
Duration	13:46 💡
OS	Linux Debian - 10.11
Browser	Firefox 98

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/e2e/origin/commands/assertions.cy.ts		1
1	cy.origin assertions > #consoleProps > .should() and .and()

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

flotwig · 2022-06-08T15:06:14Z

packages/data-context/test/unit/sources/ProjectDataSource.spec.ts

@@ -563,16 +565,11 @@ describe('_makeSpecWatcher', () => {

    specWatcher.on('add', (filePath) => allFiles.add(filePath))
    specWatcher.on('change', (filePath) => allFiles.add(filePath))
+    specWatcher.on('unlink', (filePath) => allFiles.delete(filePath))


@tgriesser I didn't think we'd be seeing this test so soon again :P I had to modify this test to pass after making so much stuff async, and this line was missing. I think the test is still testing what it's supposed to, can you verify?

lmiller1990

One comment/question around extending this to npm, the rest looks like pretty simple migrations from sync->async. 👏

Also windows is passing on this PR - did you re-run it, or did these changes reduce some flake?

lmiller1990 · 2022-06-09T04:26:37Z

.eslintrc.js

@@ -55,6 +74,16 @@ module.exports = {
        message: 'os.userInfo() will throw when there is not an `/etc/passwd` entry for the current user (like when running with --user 12345 in Docker). Do not use it unless you catch any potential errors.',
      },
    ],
+    'no-restricted-syntax': [


Wow, hope we don't need to edit this in the near future, did not realize you could do this.

Yeah. I pasted a URL there that helps you debug esquery, but even still this took me a bit to figure out.

lmiller1990 · 2022-06-09T04:30:19Z

npm/vite-dev-server/src/plugins/cypress.ts

@@ -38,6 +38,8 @@ export const Cypress = (
  const indexHtmlFile = options.cypressConfig.indexHtmlFile

  let specsPathsSet = getSpecsPathsSet(specs)
+  // TODO: use async fs methods here
+  // eslint-disable-next-line no-restricted-syntax
  let loader = fs.readFileSync(INIT_FILEPATH, 'utf8')


Does this "do not use sync FS calls" also need to be applied in npm, or just things we know are shipping in the binary (and never published separately on npm). It's possible a user is consuming these outside of Cypress (however unlikely) and they may not have access to wrapped fs module with async extensions.

Seems safest to either us things we know exist in the Node.js standard lib, or have these depend on fs-extra, or use fs.promises (didn't know about this!) to making async versions of the functions we need in each npm module.

I'd use fs.promises here if I was gonna change this. I started to, but then stopped because it actually requires a user-facing (I think?) type change to make this function async.

Since this is most likely to run in the plugins process, it should be fine to just use fs.promises, since we already run gracefulFs.gracefulify(fs) in the child process:

Not too sure this would require a user facing change (this function is internal to the Vite dev server) but sure, happy to leave it like this for now. If we do change it fs.promises seems good.

flotwig · 2022-06-09T13:43:40Z

Also windows is passing on this PR - did you re-run it, or did these changes reduce some flake?

I think I just got lucky.

lmiller1990

Seems fine, got an answer on my one question.

tbiethman

Changes are thorough 👍 I ran a few smoke tests locally and everything is working good. Also added a few sync calls to test the new eslint rule, that is very cool.

Should we log issues for the few remaining TODOs or will we just keep #22028 open for that work?

flotwig · 2022-06-15T21:04:17Z

@tbiethman I'll update #22028, I think it can encompass the remaining work on this (warn on all sync usage on a monkey-patch level, remove all sync usage)

* fix: use graceful-fs always, warn in development on sync calls * skip prop linting in some dirs * eslint rules * use AST-based lint rule instead * comment * ignore existsSync * run without nextTick * remove dev warning code * fix order * register TS first * fix tests * fix test * cover new call site * fix new test

flotwig added 5 commits June 7, 2022 13:44

fix: use graceful-fs always, warn in development on sync calls

6a70198

skip prop linting in some dirs

f057bca

eslint rules

aa6b5e2

use AST-based lint rule instead

8998557

comment

6eec60c

flotwig changed the title ~~fix: use graceful-fs always, lint and warn in development on sync FS calls~~ fix: use graceful-fs always, lint sync FS calls Jun 7, 2022

flotwig added 5 commits June 7, 2022 17:27

ignore existsSync

25efccd

run without nextTick

fcc6435

remove dev warning code

5977f26

fix order

e49864d

register TS first

180d068

fix tests

a45fcd6

flotwig changed the title ~~fix: use graceful-fs always, lint sync FS calls~~ fix: retry on EMFILE always, lint sync FS calls Jun 7, 2022

fix test

1a17036

flotwig marked this pull request as ready for review June 8, 2022 15:00

flotwig requested review from a team and tgriesser as code owners June 8, 2022 15:00

flotwig requested review from jennifer-shehane and MuazOthman and removed request for a team and jennifer-shehane June 8, 2022 15:00

flotwig commented Jun 8, 2022

View reviewed changes

lmiller1990 reviewed Jun 9, 2022

View reviewed changes

Merge branch 'develop' into block-fs-sync

1a3dc2f

flotwig requested a review from lmiller1990 June 9, 2022 13:44

flotwig requested review from estrada9166 and rachelruderman June 9, 2022 14:07

cover new call site

994e6cb

lmiller1990 approved these changes Jun 10, 2022

View reviewed changes

flotwig added 2 commits June 10, 2022 10:07

Merge branch 'develop' into block-fs-sync

a6f9ae5

Merge branch 'develop' into block-fs-sync

9ecc2f1

tbiethman self-requested a review June 15, 2022 15:45

flotwig added 2 commits June 15, 2022 16:33

Merge remote-tracking branch 'origin/develop' into block-fs-sync

e0f28f7

fix new test

9321032

tbiethman approved these changes Jun 15, 2022

View reviewed changes

lmiller1990 merged commit d01932b into develop Jun 16, 2022

lmiller1990 deleted the block-fs-sync branch June 16, 2022 04:35

cypress-bot bot mentioned this pull request Jun 16, 2022

EMFILE eventually crashes cypress open on Linux #22023

Closed

flotwig mentioned this pull request Jun 21, 2022

Warn on all runtime fs.sync* calls that need to be converted to be async and convert to async #22028

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: retry on EMFILE always, lint sync FS calls #22175

fix: retry on EMFILE always, lint sync FS calls #22175

flotwig commented Jun 7, 2022 •

edited

cypress-bot bot commented Jun 7, 2022

cypress bot commented Jun 7, 2022 •

edited

flotwig Jun 8, 2022

lmiller1990 left a comment

lmiller1990 Jun 9, 2022 •

edited

flotwig Jun 9, 2022

lmiller1990 Jun 9, 2022

flotwig Jun 9, 2022

lmiller1990 Jun 10, 2022

flotwig commented Jun 9, 2022

lmiller1990 left a comment

tbiethman left a comment

flotwig commented Jun 15, 2022 •

edited

fix: retry on EMFILE always, lint sync FS calls #22175

fix: retry on EMFILE always, lint sync FS calls #22175

Conversation

flotwig commented Jun 7, 2022 • edited

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Jun 7, 2022

cypress bot commented Jun 7, 2022 • edited

Test summary

Run details

Flakiness

flotwig Jun 8, 2022

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 Jun 9, 2022 • edited

Choose a reason for hiding this comment

flotwig Jun 9, 2022

Choose a reason for hiding this comment

lmiller1990 Jun 9, 2022

Choose a reason for hiding this comment

flotwig Jun 9, 2022

Choose a reason for hiding this comment

lmiller1990 Jun 10, 2022

Choose a reason for hiding this comment

flotwig commented Jun 9, 2022

lmiller1990 left a comment

Choose a reason for hiding this comment

tbiethman left a comment

Choose a reason for hiding this comment

flotwig commented Jun 15, 2022 • edited

flotwig commented Jun 7, 2022 •

edited

cypress bot commented Jun 7, 2022 •

edited

lmiller1990 Jun 9, 2022 •

edited

flotwig commented Jun 15, 2022 •

edited