fix: Ensure that file watchers are closed before Electron exits #22606

astone123 · 2022-06-29T21:18:42Z

Closes #22026

This PR uses a will-quit hook for Electron to ensure that we close all of the DataContext file watchers before the application exits.

See the related issue for more info.

User facing changelog

Fixed an issue where file watchers were not completely closed prior to the Cypress app quitting, causing crashes on exit in some macOS environments.

Testing:

Check out develop
Follow the repro steps in the related issue to reproduce a SIGABORT error upon exiting the application
Check out this branch
Follow the steps again and verify that the SIGABORT error will not happen

…will be a disaster currently.

…tbiethman/UNIFY-1816-prototype # Conflicts: # circle.yml # packages/data-context/src/actions/ProjectActions.ts # packages/data-context/src/sources/GitDataSource.ts

…tbiethman/UNIFY-1816-prototype

…UNIFY-1816-prototype

cypress-bot · 2022-06-29T21:18:46Z

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.

astone123 · 2022-06-29T21:21:50Z

packages/data-context/src/DataContext.ts

@@ -429,7 +429,11 @@ export class DataContext {
  }

  async destroy () {
-    const destroy = util.promisify(this.coreData.servers.gqlServer?.destroy || (() => {}))


util.promisify(() => {})

This noop didn't work because it never resolved. We replaced it with () => Promise.resolve()

astone123 · 2022-06-29T21:23:07Z

packages/server/lib/modes/interactive.ts

@@ -170,6 +173,36 @@ export = {
      makeGraphQLServer(),
    ])

+    const clearCtxAndQuit = async () => {


This file contains the bulk of the changes. Most of the other changes in this diff are related to awaiting function calls that are now async

packages/data-context/src/data/ProjectConfigManager.ts

…o/cypress into tbiethman/UNIFY-1816-prototype

cypress · 2022-06-29T21:51:36Z

Test summary

37721 • 0 • 456 • 0 • 10

Run details


Project	cypress
Status	Passed
Commit	`0a3e614`
Started	Jul 17, 2022 7:17 PM
Ended	Jul 17, 2022 7:33 PM
Duration	16:04 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	xhr.cy.js	4
1	... > logs request + response headers
2	... > logs Method, Status, URL, and XHR
3	... > logs request + response headers
4	... > logs Method, Status, URL, and XHR
	navigation.cy.js	1
1	src/cy/commands/navigation > #visit > window immediately resolves and doesn't reload when visiting the same URL with hashes
This comment includes only the first 5 flaky tests. See all 10 flaky tests in the Cypress Dashboard.

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

packages/server/test/integration/cypress_spec.js

lmiller1990

Sorry it took so long to review, I missed this one. I left a few tiny comments, once those are good let's ship it.

lmiller1990 · 2022-07-12T01:20:08Z

packages/server/lib/modes/interactive.ts

@@ -170,6 +173,36 @@ export = {
      makeGraphQLServer(),
    ])

+    const clearCtxAndQuit = async () => {
+      try {
+        await clearCtx()


There is a comment saying we should not call this in runtime: https://github.com/cypress-io/cypress/pull/22606/files#diff-1827debd8088884005aba0e14a94e1e6815ceb1ad567db970f9f327d9feb7c9dR27

If we are doing this, I think we need to change the comment?

Also, maybe a comment here explaining why we clearCtx before closing? Could link to the relevant GH issue

lmiller1990 · 2022-07-12T01:20:57Z

packages/server/lib/modes/run.js

@@ -1650,6 +1650,7 @@ module.exports = {
  async run (options, loading = Promise.resolve()) {
    if (require('../util/electron-app').isRunningAsElectronProcess({ debug })) {
      const app = require('electron').app
+      const { clearCtx } = require('@packages/data-context')


Was there a reason to inline this require instead of putting it at the top?

Will make an eventual ESM convert easier.

The existing data-context require used to be at root but was changed with https://github.com/cypress-io/cypress/pull/20932/files#diff-58fd4a8e28c13b52b19c42346eade66ed1de5a8b139541dff4e94fef42c3c701R666-R667. I assume due to how the run script is getting loaded?

Regardless, I am going to revert the changes to this file for run mode. I don't think the process.exit handler is deterministic here, just like it wasn't before in the ProjectLifecycleManager. And because we don't initialize watchers in run mode, the teardown isn't necessary prior to exit.

packages/server/test/integration/cypress_spec.js

…tbiethman/UNIFY-1816-prototype # Conflicts: # packages/data-context/src/data/ProjectLifecycleManager.ts

…s://github.com/cypress-io/cypress into tbiethman/UNIFY-1816-prototype

tbiethman · 2022-07-13T17:52:09Z

@astone123 @lmiller1990 I have addressed the current PR feedback and pushed a few new commits.

The biggest change was addressing the TODO regarding closing the file watchers when the ProjectConfigManager errored. I was able to keep closing these by untangling some of the circular logic between the ProjectLifecycleManager and the ProjectConfigManager. You can that see in the diff. Doing it this way allows us to keep the lifecycle manager error handling functionally equivalent while allowing for async handling within the config manager itself. I'm happy to answer more questions around that, otherwise I think we're good to go for a final review.

ryanthemanuel · 2022-07-13T18:48:00Z

packages/data-context/src/data/ProjectLifecycleManager.ts

-  destroy () {
-    this.resetInternalState()
-    // @ts-ignore
-    process.removeListener('exit', this.onProcessExit)


Do we not need to remove this anymore?

Nope, we no longer add the listener in the first place.

lmiller1990

My comments are addressed. Just need CI to pass and we can merge?

cypress-bot · 2022-07-19T17:31:52Z

Released in 10.3.1.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.3.1, please open a new issue.

tbiethman and others added 18 commits May 25, 2022 16:11

Adding prototype before-quit handler to handle async teardown.

41c6f9b

Getting binary builds

dfbc9c4

Let's try this then

efb8f5d

Working the async changes backwards, hope I got them all. Unit tests …

ba2b979

…will be a disaster currently.

Merge branch 'develop' of https://github.com/cypress-io/cypress into …

9a8efe9

…tbiethman/UNIFY-1816-prototype # Conflicts: # circle.yml # packages/data-context/src/actions/ProjectActions.ts # packages/data-context/src/sources/GitDataSource.ts

Actually getting build artifacts for testing

66fe0b0

Moving logic to server interactive/run

d2077a1

Merge branch 'develop' of https://github.com/cypress-io/cypress into …

4e080d4

…tbiethman/UNIFY-1816-prototype

fix: Fix some tests

1fe4074

fix: Revert changes to circle config

a5aaf35

fix: Merge develop in

c8cacc4

fix: Fix some tests

7323fb3

Merge branch 'develop' of github.com:cypress-io/cypress into tbiethman/…

7092f15

…UNIFY-1816-prototype

fix: Fix more tests

231f5fe

fix: Remove dead code comment

362fb29

fix: Fix ProjectDataSource tests

0c4c399

Merge branch 'develop' of github.com:cypress-io/cypress into tbiethman/…

64ea777

…UNIFY-1816-prototype

fix: Add comment prefix

ee7e053

Merge branch 'develop' into tbiethman/UNIFY-1816-prototype

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

07e6bdf

astone123 commented Jun 29, 2022

View reviewed changes

fix: Remove comment and unnecessary async

a5e919d

astone123 commented Jun 29, 2022

View reviewed changes

packages/data-context/src/data/ProjectConfigManager.ts Outdated Show resolved Hide resolved

Merge branch 'tbiethman/UNIFY-1816-prototype' of github.com:cypress-i…

b9977cd

…o/cypress into tbiethman/UNIFY-1816-prototype

astone123 commented Jun 30, 2022

View reviewed changes

packages/server/test/integration/cypress_spec.js Outdated Show resolved Hide resolved

astone123 marked this pull request as ready for review June 30, 2022 18:54

astone123 requested review from a team as code owners June 30, 2022 18:54

astone123 requested a review from ZachJW34 June 30, 2022 19:39

astone123 added 2 commits June 30, 2022 15:43

Merge branch 'develop' into tbiethman/UNIFY-1816-prototype

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

24be2e4

fix: Build Mac binary

8e0ca94

lmiller1990 reviewed Jul 12, 2022

View reviewed changes

tbiethman added 10 commits July 12, 2022 11:40

Reverting run changes

586acde

Addressing PR comments. Cleaning up a few naming quibbles I had.

4cf7dad

Addressing TODO regarding onLoadError watcher cleanup.

e4c8c8b

Correcting catch

33d1e8b

Merge branch 'develop' of https://github.com/cypress-io/cypress into …

7d169b1

…tbiethman/UNIFY-1816-prototype # Conflicts: # packages/data-context/src/data/ProjectLifecycleManager.ts

Fighting some unref errors on these catches

6eadcda

Reverting making this private en lieu of binding

4676a01

Should have left these as instances, whoops

fb0f5dd

Merge branches 'tbiethman/UNIFY-1816-prototype' and 'develop' of http…

3b28537

…s://github.com/cypress-io/cypress into tbiethman/UNIFY-1816-prototype

Removing unnecessary test that was previously skipped

47d859c

Adding a couple cheap unit tests for the new interactive mode behavior

d4464b7

ryanthemanuel reviewed Jul 13, 2022

View reviewed changes

ryanthemanuel approved these changes Jul 13, 2022

View reviewed changes

tbiethman added 3 commits July 13, 2022 14:34

Merge branch 'develop' into tbiethman/UNIFY-1816-prototype

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

9135c59

Merge branch 'develop' into tbiethman/UNIFY-1816-prototype

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

a4d0889

Merge branch 'develop' into tbiethman/UNIFY-1816-prototype

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

aed05b9

lmiller1990 self-requested a review July 15, 2022 05:51

lmiller1990 approved these changes Jul 15, 2022

View reviewed changes

tbiethman added 3 commits July 15, 2022 12:05

Merge branch 'develop' into tbiethman/UNIFY-1816-prototype

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

8f03f16

Merge branch 'develop' into tbiethman/UNIFY-1816-prototype

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

2dee8ba

Merge branch 'develop' into tbiethman/UNIFY-1816-prototype

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

0a3e614

tbiethman merged commit 1b68869 into develop Jul 17, 2022

tbiethman deleted the tbiethman/UNIFY-1816-prototype branch July 17, 2022 20:57

cypress-bot bot locked as resolved and limited conversation to collaborators Jul 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Ensure that file watchers are closed before Electron exits #22606

fix: Ensure that file watchers are closed before Electron exits #22606

astone123 commented Jun 29, 2022 •

edited by tbiethman

Loading

Uh oh!

cypress-bot bot commented Jun 29, 2022

Uh oh!

astone123 Jun 29, 2022

Uh oh!

astone123 Jun 29, 2022

Uh oh!

Uh oh!

cypress bot commented Jun 29, 2022 •

edited

Loading

Uh oh!

Uh oh!

lmiller1990 left a comment

Uh oh!

lmiller1990 Jul 12, 2022

Uh oh!

lmiller1990 Jul 12, 2022

Uh oh!

tbiethman Jul 12, 2022

Uh oh!

Uh oh!

tbiethman commented Jul 13, 2022

Uh oh!

ryanthemanuel Jul 13, 2022

Uh oh!

tbiethman Jul 13, 2022

Uh oh!

lmiller1990 left a comment

Uh oh!

cypress-bot bot commented Jul 19, 2022

Uh oh!

fix: Ensure that file watchers are closed before Electron exits #22606

fix: Ensure that file watchers are closed before Electron exits #22606

Conversation

astone123 commented Jun 29, 2022 • edited by tbiethman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User facing changelog

Uh oh!

cypress-bot bot commented Jun 29, 2022

Uh oh!

astone123 Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

astone123 Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cypress bot commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test summary

Run details

Flakiness

Uh oh!

Uh oh!

lmiller1990 left a comment

Choose a reason for hiding this comment

Uh oh!

lmiller1990 Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

lmiller1990 Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

tbiethman Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tbiethman commented Jul 13, 2022

Uh oh!

ryanthemanuel Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

tbiethman Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

lmiller1990 left a comment

Choose a reason for hiding this comment

Uh oh!

cypress-bot bot commented Jul 19, 2022

Uh oh!

astone123 commented Jun 29, 2022 •

edited by tbiethman

Loading

cypress bot commented Jun 29, 2022 •

edited

Loading