FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: minimize test-http-get-pipeline-problem by Trott · Pull Request #5728 · nodejs/node · GitHub

/ node Public

test: minimize test-http-get-pipeline-problem - #5728

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fix-pipeline
ConversationCommits (1)ChecksFiles changed
Closed

test: minimize test-http-get-pipeline-problem#5728
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fix-pipeline

Conversation

Trott commented Mar 15, 2016

Copy link
Copy Markdown
Member

Pull Request check-list

Please make sure to review and check all of these items:

  • 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][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

test, http

Description of change

Reduce resoures required by test. Fixes test flakiness on Raspberry Pi in CI.

Clarify comment explaining source of test and what the test is looking
for.

Fixes: #5725

test: minimize test-http-get-pipeline-problem
100442f 
Reduce resoures required by test.

Clarify comment explaining source of test and what the test is looking
for.

Fixes: nodejs#5725
Trott added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. arm Issues and PRs related to the ARM architecture. labels Mar 15, 2016

Trott commented Mar 15, 2016

Copy link
Copy Markdown
Member Author

Stress test showing flakiness on current master: https://ci.nodejs.org/job/node-stress-single-test/560/nodes=pi2-raspbian-wheezy/console

Trott commented Mar 15, 2016

Copy link
Copy Markdown
Member Author

For context:

Here's a gist from @mikeal who originally reported the bug: https://gist.githubusercontent.com/mikeal/864727/raw/561f3b665df0faff6d3ee9601ef31cc3caf0376f/gistfile1.js

That code fails on Node.js 0.6.0 and succeeds in 0.6.21.

If we change it to this, it still failsin Node.js 0.6.0 and succeeds in 0.6.21:

var http = require('http');
var fs = require('fs');
var parseUrl = require('url').parse;

http.globalAgent.maxSockets = 1;

var i = 10;
while(i > 0) {
  (function () {
    var x = i;
    var u = parseUrl("http://farm4.static.flickr.com/3557/3670453345_c38d482efb_m.jpg")

    var c = http.request(
      { host:u.hostname
      , port:u.port
      , method: 'GET'
      , path: u.pathname
      , headers: {host:u.hostname}
      }
      , function (res) {
        console.log(x)
        res.pipe(fs.createWriteStream(x + ".jpg"))
    })
    c.end();
    i--;
  })()
}

The key is that http.globalAgent.maxSockets needs to be less than i to trip the bug. So, that's why we are able to reduce the number of sockets in this test the way we do in this PR.

Trott commented Mar 16, 2016

Copy link
Copy Markdown
Member Author

Stress test to hopefully show that this is not flaky. Hope I didn't typo in the parameters...

https://ci.nodejs.org/job/node-stress-single-test/563/nodes=pi2-raspbian-wheezy/console

Trott commented Mar 18, 2016

Copy link
Copy Markdown
Member Author

Bump. /cc @nodejs/testing

santigimeno commented Mar 18, 2016

Copy link
Copy Markdown
Member

LGTM

Trott mentioned this pull request Mar 19, 2016
Closed

cjihrig commented Mar 20, 2016

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment

jasnell commented Mar 21, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell pushed a commit that referenced this pull request Mar 21, 2016
test: minimize test-http-get-pipeline-problem
ccb7b45 
Reduce resoures required by test.

Clarify comment explaining source of test and what the test is looking
for.

Fixes: #5725
PR-URL: #5728
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Mar 21, 2016

Copy link
Copy Markdown
Member

Landed in ccb7b45

jasnell closed this Mar 21, 2016
Fishrock123 mentioned this pull request Mar 22, 2016
Merged
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
test: minimize test-http-get-pipeline-problem
b352cc7 
Reduce resoures required by test.

Clarify comment explaining source of test and what the test is looking
for.

Fixes: #5725
PR-URL: #5728
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
test: minimize test-http-get-pipeline-problem
380785b 
Reduce resoures required by test.

Clarify comment explaining source of test and what the test is looking
for.

Fixes: #5725
PR-URL: #5728
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
test: minimize test-http-get-pipeline-problem
7e45d4f 
Reduce resoures required by test.

Clarify comment explaining source of test and what the test is looking
for.

Fixes: #5725
PR-URL: #5728
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott deleted the fix-pipeline branch January 13, 2022 22:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

arm Issues and PRs related to the ARM architecture. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate flaky test-http-get-pipeline-problem

5 participants

Back | FazBrowse Home | New Git URL