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

[Regression in 14.1.0 - Windows] stdout is sometimes empty #33166

Closed
nicolo-ribaudo opened this issue Apr 30, 2020 · 148 comments
Closed

[Regression in 14.1.0 - Windows] stdout is sometimes empty #33166

nicolo-ribaudo opened this issue Apr 30, 2020 · 148 comments
Labels
stream Issues and PRs related to the stream subsystem. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.

Comments

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Apr 30, 2020

  • Version: 14.1.0
  • Platform: Windows 64-bit on VirtualBox and on Travis CI
  • Subsystem: N/A

Bug description

After the Node.js 14.1.0 release, Babel's CI tests on Windows started failing. The failures are all related to @babel/cli and @babel/node: we run those CLI programs, capture their output (from stdout) and compare it with the expected output.

Sometimes, the generated stdout is empty: you can see an example here or here. In this CI log you can also see some error messages coming from V8 internals in @babel/node tests, but I don't know if it's the same problem.

How often does it reproduce? Is there a required condition?

My guess is that every test has about 1% chance of failing. However, we have ~100 tests for those 2 packages so something is failing more often than not.

What steps will reproduce the bug?

I couldn't find a small and isolated reproduction example. I'll keep trying to create one, but here is what I have for now.
Also, building Babel on Windows is painful and I couldn't run the full test suite. However, I managed to reproduce the bug.

I'm running these commands using Powershell, Nodej.s 14.1.0 and Yarn 1.22

# Clone the babel repository
git clone https://github.com/babel/babel.git .

# Install deps
yarn
yarn lerna bootstrap # This takes a while

# Build Babel (this won't build everything, but enough packages to show the bug)
yarn gulp build-no-bundle

# Run the @babel/cli and @babel/node tests
yarn jest --runInBand babel-node babel-cli

If you don't see the last command failing, try running it 2 or three times. The --runInBand option isn't necessary to reproduce the problem, but it disables Jest's workers so it removes one possible cause.

What is the expected behavior?

Tests should pass

What do you see instead?

An example of output failing locally is this:

PS C:\Users\Nicolò\Downloads\babel-master\babel-master> yarn jest --runInBand babel-node babel-cli
yarn run v1.22.4
$ C:\Users\Nicolò\Downloads\babel-master\babel-master\node_modules\.bin\jest --runInBand babel-node babel-cli
 FAIL  packages/babel-cli/test/index.js (33.099s)
   bin/babel  --only glob

    "C:\Users\Nicolò\Downloads\babel-master\babel-master\packages\babel-cli\lib\babel" "--presets" "C:\Users\Nicolò\Downloads\babel-master\babel-master\packages\babel-preset-react" "--plugins" "C:\Users\Nicolò\Downloads\babel-master\babel-master\packages\babel-plugin-transform-arrow-functions,C:\Users\Nicolò\Downloads\babel-master\babel-master\packages\babel-plugin-transform-strict-mode,C:\Users\Nicolò\Downloads\babel-master\babel-master\packages\babel-plugin-transform-modules-commonjs" "src" "--out-dir" "lib" "--only" "**/*.foo.js" "--verbose": expect(received).toBe(expected) // Object.is equality

    Expected: "src/a.foo.js -> lib/a.foo.js
    src/baz/b.foo.js -> lib/baz/b.foo.js
    Successfully compiled 2 files with Babel."
    Received: ""

      85 |       expect(stdout).toContain(expectStdout);
      86 |     } else {
    > 87 |       expect(stdout).toBe(expectStdout);
         |                      ^
      88 |     }
      89 |   } else if (stdout) {
      90 |     throw new Error("stdout:\n" + stdout);

      at assertTest (packages/babel-cli/test/index.js:87:22)
      at ChildProcess.<anonymous> (packages/babel-cli/test/index.js:152:9)

 PASS  packages/babel-node/test/index.js (22.924s)

Test Suites: 1 failed, 1 passed, 2 total
Tests:       1 failed, 89 passed, 90 total
Snapshots:   0 total
Time:        56.088s, estimated 62s
Ran all test suites matching /babel-node|babel-cli/i.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Additional information

@lpinca
Copy link
Member

lpinca commented Apr 30, 2020

cc: @nodejs/streams @ronag

@lpinca lpinca added the stream Issues and PRs related to the stream subsystem. label Apr 30, 2020
@mcollina
Copy link
Member

Can you confirm this happening only on Windows? This is widely unexpected.

@ronag
Copy link
Member

ronag commented Apr 30, 2020

I can't reproduce on OSX. @nicolo-ribaudo any chance you could bisect? I don't have a Windows machine at the moment.

@ronag
Copy link
Member

ronag commented Apr 30, 2020

I'm unsure where to go with the V8 errors. @addaleax maybe?

@nicolo-ribaudo
Copy link
Contributor Author

Yes, I can only reproduce this on windows (not the V8 errors), only the empty stdouts. I'll try to bisect.

@addaleax
Copy link
Member

@ronag I’m not sure… Stack traces on Windows are basically garbage unless you take some extra steps (that I don’t know how to take), and given that the error message isn’t particularly helpful here either – there are over 2000 instances of UNREACHABLE(); in V8 – I’m not quite sure where to start here.

/cc @nodejs/v8 @nodejs/platform-windows

@nicolo-ribaudo
Copy link
Contributor Author

Update: this might not be a Node.js bug.

Today I made two local clones of the Babel repository: one ~6 hours ago and I can consistently reproduce the errors there, and one ~2 hours ago where I cannot reproduce the errors. This makes me think that maybe it's a bug in a dependency that didn't support Node.js 14.1.0 on windows, and it was recently updated to fix the bug.

Even if there was a bug in a dependency I don't think that it should trigger the V8 internal error, but I cannot reproduce that one anyway 🤷

I'll try to re-run the build on Travis CI a bunch of times, and see if I can reproduce it there.

@ronag
Copy link
Member

ronag commented Apr 30, 2020

This makes me think that maybe it's a bug in a dependency that didn't support Node.js 14.1.0 on windows, and it was recently updated to fix the bug.

Can you diff the dependencies?

@nicolo-ribaudo
Copy link
Contributor Author

Yeah I'm trying. I'm on a Windows VM and I haven't used Windows for a while, so it might take some time 😛

Also, another thing I noticed: in the folder where I can (consistently!) reproduce the problem, I can only reproduce it when using Node.js 14.1.0. If I use a build form master (4b2d958) it doesn't fail.

@ronag
Copy link
Member

ronag commented Apr 30, 2020

If I use a build form master (4b2d958) it doesn't fail.

Yea, a bisect would be very useful.

@nicolo-ribaudo
Copy link
Contributor Author

I gave up for today, I will continue tomorrow.
Also, I noticed that Jest's own tests have the same problem: https://github.com/facebook/jest/pull/9934/checks?check_run_id=633574558 (cc @SimenB)

@dr-js
Copy link
Contributor

dr-js commented May 1, 2020

Not sure if it's related, I got win10 + nodejs@14 + babel on CI with flaky ELIFECYCLE/3221225477 exit code in one of my repo: nodejs/help#2660

Update:

I've reduced CI exit test case to some of my source files and babel + 2 babel plugins
The code is at: https://github.com/dr-js/dr-dev/tree/8e111ec5194adc3e159db2d3bc3594f8587dc230
The latest CI fail: https://github.com/dr-js/dr-dev/actions/runs/92784081

Update 2:

With more debug log found the exit happen before the @babel/env plugin gets load, after minify-replace and module-resolver loaded.
Related CI run: https://github.com/dr-js/dr-dev/actions/runs/92845322

@SimenB
Copy link
Member

SimenB commented May 1, 2020

Yeah, Jest is failing often on Node 14.1.0 on Windows with empty stdout in one test or the other almost every test run. The V8 thing is actually in the latest build on master: https://github.com/facebook/jest/runs/634830924

@nicolo-ribaudo
Copy link
Contributor Author

Ok, have no idea where to start bisecting. I can still consistently reproduce the failures with the Node.js 14.1.0 installed with the installer, but not if I build 9ffd8e7.

@ronag ronag added v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform. labels May 1, 2020
@mcollina
Copy link
Member

mcollina commented May 2, 2020

Ok, have no idea where to start bisecting. I can still consistently reproduce the failures with the Node.js 14.1.0 installed with the installer, but not if I build 9ffd8e7.

@nodejs/releasers can you check if there could be any differences between what is available via the installer and what was tagged?

@targos
Copy link
Member

targos commented May 2, 2020

I think there's no way to be sure it's the promoted one, but I can confirm that the latest release build was done with commit 9ffd8e7
Refs: https://ci-release.nodejs.org/job/iojs+release/5995/
The times of the file on the server and in the build logs also match.

@mcollina
Copy link
Member

mcollina commented May 2, 2020

@jasnell I know you have a beefy windows machine, could you check this out and run a bisect?

Yeah, Jest is failing often on Node 14.1.0 on Windows with empty stdout in one test or the other almost every test run. The V8 thing is actually in the latest build on master: https://github.com/facebook/jest/runs/634830924

@SimenB This is worrysome. Are you using any wasm inside Jest?

@targos
Copy link
Member

targos commented May 2, 2020

I'm building the release commit on my machine (Win10, VS2019)

@SimenB
Copy link
Member

SimenB commented May 2, 2020

@mcollina nope, only experimental stuff we ship is the VM ESM APIs, but that code is guarded behind a check that vm.SyntheticModule is defined, which is flagged.

EDIT: That said, some dependency might? Seems unlikely, though - node 14.0 behaved fine, and both mac and linux on 14.1 behaves fine

@targos
Copy link
Member

targos commented May 2, 2020

Well, we'd need some more simple and reliable way to reproduce this.
I cloned Jest and tried to run e2e\__tests__\toThrowErrorMatchingInlineSnapshot.test.ts multiple times with my local build and the release but it never errors.

@mcollina
Copy link
Member

mcollina commented May 2, 2020

That said, some dependency might? Seems unlikely, though - node 14.0 behaved fine, and both mac and linux on 14.1 behaves fine

The V8 stacktrace you posted shows a failure within some code in the wasm namespace in V8.

Hopefully @targos can shed some light on this.

@targos
Copy link
Member

targos commented May 2, 2020

Commits between 14.0.0 and 14.1.0: v14.0.0...v14.1.0
3 of them to deps/v8:

@nicolo-ribaudo
Copy link
Contributor Author

I'm now trying to remove as much code as possible from my local failing clone of Babel to reduce the number of possible error causes.

@mcollina
Copy link
Member

mcollina commented May 4, 2020

This seems to be not babel specific: https://github.com/pinojs/pino/runs/642073186. Again, this is consistently flaky :(.

@targos did you run the tests in Linux WSL or using the normal bin? I have a hunch that Github CI is using WSL.

@targos
Copy link
Member

targos commented May 4, 2020

I used the normal bin in PowerShell

@mcollina
Copy link
Member

mcollina commented May 4, 2020

Would you mind to try out if it works in WSL?

@targos
Copy link
Member

targos commented May 4, 2020

@mcollina what exactly do you want me to try?
BTW I cloned pino, npm i, npm t with Node.js 14.1.0 on Windows and all tests pass 😕

@SimenB
Copy link
Member

SimenB commented May 15, 2020

The revert will not be backported to v13, right? Only applicable to v14? The only code I've got that uses it is on an unpushed branch on my own machine, so I ask more to manage my own expectations rather than for fear of breakage

@devsnek
Copy link
Member

devsnek commented May 15, 2020

did this only ever reproduce with builds directly from our windows release build servers?

@nicolo-ribaudo
Copy link
Contributor Author

Also locally: #33166 (comment)

codebytere pushed a commit that referenced this issue May 16, 2020
This reverts commit 74c393d.

Fixes: #33166

PR-URL: #33364
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Flarna
Copy link
Member

Flarna commented May 19, 2020

Would be nice if someone who was able to reproduce this test with 14.3.0.

@mcollina
Copy link
Member

I cannot reproduce the problem with Node v14.3.0. It's fully solved for me.

@nicolo-ribaudo
Copy link
Contributor Author

I broke my laptop so I cannot test it locally, but as soon as Travis supports Node.js 14.3.0 I can test it on our CI.

@SimenB
Copy link
Member

SimenB commented May 20, 2020

Could #31860 be reopened? Or is the un-reverting tracked somewhere else?

@bzoz
Copy link
Contributor

bzoz commented May 20, 2020

Would be nice if someone who was able to reproduce this test with 14.3.0.

Can confirm, built 14.3.0 locally, the issue no longer reproduces.

@SimenB
Copy link
Member

SimenB commented Jun 24, 2020

This still happens using node 14.4.0, fwiw. See https://github.com/facebook/jest/pull/10188/checks?check_run_id=802977045

image

image

@mcollina
Copy link
Member

Maybe @orangemocha can help here.

@orangemocha
Copy link
Contributor

Looping in @bzoz @joaocgreis @jaimecbernardo

@bzoz
Copy link
Contributor

bzoz commented Jun 29, 2020

Sorry, I cannot reproduce with 14.4.0.

dr-js added a commit to dr-js/dr-dev that referenced this issue Jul 11, 2020
notable change:
- break: move `getPackageTgzName` to `@dr-js/node` as `toPackageTgzName` in `module/Software/npm`
- break: promote `common/terminalColor`, `node/npm/path`, and `parsePackageNameAndVersion` from `node/npm/npxLazy` to `@dr-js/node`
- break: use `getGitBranch|getGitCommitHash` from `@dr-js/node`
- break: use `createTest` to init test in `common/test`
- break: longer default test timeout
- break: use `nodejs@14` instead of `nodejs@13` for CI test
- break: use `dr-dev -E` instead of `cross-env`
- break: update to `eslint-config-almost-standard-v14`
- break: update to `eslint-config-almost-standard-jsx-v8`
- break: use `getSourceJsFileListFromPathList` instead of `getScriptFileListFromPathList` from `node/filePreset`
- break: use `collectSourceJsRouteMap` instead of `collectSourceRouteMap` from `node/export/parsePreset`
- break: pass `pathAutoLicenseFile` instead of `pathLicenseFile` to `initOutput()` from `output`
- add: `@dr-js/node` as dependency
- add: `@dr-js/dev-eslint` config
- add: `clearOutput` to `output`
- add: `wrapTestScriptStringToHTML` to `puppeteer`
- add: cleaner `isTest` script
- add: verify rule
- add: `source/node/preset` for keep preset path naming rule in one place
- add: support `testUrl` for `testWithPuppeteer`
- fix: default `pathInfoFilter` in `collectSourceJsRouteMap`
- fix: code lint
- fix: build script clean `README.md` file
- bin: add: `exec` and `exec-load` mode
- ci: mark `windows-latest + 14.x` as unstable. check: nodejs/help#2660 and nodejs/node#33166
- ci: test on all 3 platform
- ci: update GitHub CI: https://github.blog/changelog/2020-04-15-github-actions-sets-the-ci-environment-variable-to-true/
- ci: update `INIT#.github#workflows#ci-test.yml`
- script sort
- package update
dr-js added a commit to dr-js/dr-dev that referenced this issue Jul 11, 2020
notable change:
- break: move `getPackageTgzName` to `@dr-js/node` as `toPackageTgzName` in `module/Software/npm`
- break: promote `common/terminalColor`, `node/npm/path`, and `parsePackageNameAndVersion` from `node/npm/npxLazy` to `@dr-js/node`
- break: use `getGitBranch|getGitCommitHash` from `@dr-js/node`
- break: use `createTest` to init test in `common/test`
- break: longer default test timeout
- break: use `nodejs@14` instead of `nodejs@13` for CI test
- break: use `dr-dev -E` instead of `cross-env`
- break: update to `eslint-config-almost-standard-v14`
- break: update to `eslint-config-almost-standard-jsx-v8`
- break: use `getSourceJsFileListFromPathList` instead of `getScriptFileListFromPathList` from `node/filePreset`
- break: use `collectSourceJsRouteMap` instead of `collectSourceRouteMap` from `node/export/parsePreset`
- break: pass `pathAutoLicenseFile` instead of `pathLicenseFile` to `initOutput()` from `output`
- add: `@dr-js/node` as dependency
- add: `@dr-js/dev-eslint` config
- add: `clearOutput` to `output`
- add: `wrapTestScriptStringToHTML` to `puppeteer`
- add: cleaner `isTest` script
- add: verify rule
- add: `source/node/preset` for keep preset path naming rule in one place
- add: support `testUrl` for `testWithPuppeteer`
- fix: default `pathInfoFilter` in `collectSourceJsRouteMap`
- fix: code lint
- fix: build script clean `README.md` file
- bin: add: `exec` and `exec-load` mode
- ci: mark `windows-latest + 14.x` as unstable. check: nodejs/help#2660 and nodejs/node#33166
- ci: test on all 3 platform
- ci: update GitHub CI: https://github.blog/changelog/2020-04-15-github-actions-sets-the-ci-environment-variable-to-true/
- ci: update `INIT#.github#workflows#ci-test.yml`
- script sort
- package update
devsnek added a commit to devsnek/node that referenced this issue Sep 29, 2020
dr-js added a commit to dr-js/dr-dev that referenced this issue Oct 12, 2020
notable change:
- break: use `webpack@5`
- break: use `@dr-js/*@0.4.0-dev.*`
- ci: unmark `windows-latest + 14.x` as unstable. issue may be fixed, check: nodejs/help#2660 and nodejs/node#33166
- script sort
- package update
devsnek added a commit to devsnek/node that referenced this issue Feb 5, 2021
devsnek added a commit to devsnek/node that referenced this issue Feb 5, 2021
devsnek added a commit that referenced this issue Feb 5, 2021
This reverts commit 2d5d773.

See: #32985
See: #33364
See: #33166
Fixes: #31860

PR-URL: #35431
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
danielleadams pushed a commit that referenced this issue Feb 16, 2021
This reverts commit 2d5d773.

See: #32985
See: #33364
See: #33166
Fixes: #31860

PR-URL: #35431
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
@targos
Copy link
Member

targos commented Nov 20, 2021

It seems like this is fixed now. Please reopen if I'm mistaken.

@targos targos closed this as completed Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.