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

test: Add test for piping large input from stdin #5949

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included? (This is a regression test)

Affected core subsystem(s)

test

Description of change

Check that piping a large chunk of data from process.stdin into process.stdout does not lose any data by verifying that the output has the same size as the input.

This is a regression test for #5927 and fails for the commits in the range [ace1009..89abe86).

@Fishrock123 Fishrock123 added test Issues and PRs related to the tests. process Issues and PRs related to the process subsystem. labels Mar 29, 2016

const child = spawn(process.execPath, [__filename, 'child'], { stdio: 'pipe' });

const expectedBytes = 1024 * 1024;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, is there any significance to this number? Will this reproduce the issue on all operating systems?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 29, 2016

LGTM with a question.

@addaleax
Copy link
Member Author

@cjihrig Answering your questions outside of the diff since more people might ask themselves that:

I think any input size above 65536 bytes works, but when adding an additional listener via .on('data', …) to process.stdin, the test became a little flaky (sometimes up to 5 blocks of 65536 bytes each would “make it through” the child process).

I can only verify that this test works on Linux. But as far as I can tell, the underlying issue depended solely on the relative timing of pause() and resume() calls (and the emission of the corresponding events), so I think there’s a good chance this does also catch the bug on other OSes, too.

Check that piping a large chunk of data from `process.stdin`
into `process.stdout` does not lose any data by verifying that
the output has the same size as the input.

This is a regression test for nodejs#5927 and fails for the commits
in the range [ace1009..89abe86).
@addaleax addaleax force-pushed the test-regression-5927 branch from f13bddb to 08214ba Compare March 29, 2016 22:06
@addaleax
Copy link
Member Author

Updated with your suggestion.

@evanlucas
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/2091/

@mcollina
Copy link
Member

LGTM

@bnoordhuis
Copy link
Member

LGTM and thanks, landed in 761787b.

@bnoordhuis bnoordhuis closed this Mar 30, 2016
bnoordhuis pushed a commit that referenced this pull request Mar 30, 2016
Check that piping a large chunk of data from `process.stdin`
into `process.stdout` does not lose any data by verifying that
the output has the same size as the input.

This is a regression test for #5927 and fails for the commits
in the range [ace1009..89abe86).

PR-URL: #5949
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@addaleax addaleax deleted the test-regression-5927 branch March 30, 2016 09:17
@jasnell
Copy link
Member

jasnell commented Mar 30, 2016

@thealphanerd @nodejs/lts ... while this test may not be entirely applicable to v4 but it may still be worth having.

@Fishrock123
Copy link
Contributor

Should probably go onto lts so we can catch anything similar there

@MylesBorins
Copy link
Contributor

@jasnell +1

evanlucas pushed a commit that referenced this pull request Mar 30, 2016
Check that piping a large chunk of data from `process.stdin`
into `process.stdout` does not lose any data by verifying that
the output has the same size as the input.

This is a regression test for #5927 and fails for the commits
in the range [ace1009..89abe86).

PR-URL: #5949
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
Check that piping a large chunk of data from `process.stdin`
into `process.stdout` does not lose any data by verifying that
the output has the same size as the input.

This is a regression test for #5927 and fails for the commits
in the range [ace1009..89abe86).

PR-URL: #5949
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
Check that piping a large chunk of data from `process.stdin`
into `process.stdout` does not lose any data by verifying that
the output has the same size as the input.

This is a regression test for #5927 and fails for the commits
in the range [ace1009..89abe86).

PR-URL: #5949
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins
Copy link
Contributor

Landed in v4.x-staging as 1994ac0 with a slight modification as buffer.alloc does not exist in that release stream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants