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: fix pummel/test-net-connect-memleak #21658

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 4, 2018

A loop that generates a long array is resulting in a RangeError. Moving
to Array.prototype.fill() along with the ** operator instead of using a
loop fixes the issue.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

A loop that generates a long array is resulting in a RangeError. Moving
to Array.prototype.fill() along with the ** operator instead of using a
loop fixes the issue.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 4, 2018
@Trott
Copy link
Member Author

Trott commented Jul 4, 2018

Since pummel tests are not run in CI, a lite CI is likely sufficient for this. (It will lint the test at least.)

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

The test could use a larger makeover and be moved to parallel, I think. With async_hooks, we now have a public API to make GC observable from userland, so we can avoid using a huge amount of memory just for that :)

@Trott
Copy link
Member Author

Trott commented Jul 4, 2018

@addaleax I pondered seeing what would happen if I moved it to sequential as it only took four seconds to run on my laptop, but I worried that assert(reclaimed > 128 * 1024); // It's around 256 MB on x64. would be unreliable across platforms.

@addaleax
Copy link
Member

addaleax commented Jul 4, 2018

@Trott Yeah, sorry for being unclear – I’d only suggest moving out of pummel if we get rid of the huge array that we use here.

(If you want, it’s totally fine to leave a TODO here and/or ping me when this lands 🙂 )

@Trott
Copy link
Member Author

Trott commented Jul 5, 2018

@Trott Yeah, sorry for being unclear

@addaleax Oh no, you were clear. Sorry. I was just mentioning my own considerations for moving it, but yeah, it's all irrelevant if we move to async_hooks like you suggested. TBH, in retrospect, I probably shouldn't have even left the comment. :-D

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@lpinca lpinca mentioned this pull request Jul 6, 2018
2 tasks
@Trott
Copy link
Member Author

Trott commented Jul 6, 2018

Landed in ff958ad

Trott added a commit to Trott/io.js that referenced this pull request Jul 6, 2018
A loop that generates a long array is resulting in a RangeError. Moving
to Array.prototype.fill() along with the ** operator instead of using a
loop fixes the issue.

PR-URL: nodejs#21658
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@Trott Trott closed this Jul 6, 2018
targos pushed a commit that referenced this pull request Jul 8, 2018
A loop that generates a long array is resulting in a RangeError. Moving
to Array.prototype.fill() along with the ** operator instead of using a
loop fixes the issue.

PR-URL: #21658
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jul 9, 2018
A loop that generates a long array is resulting in a RangeError. Moving
to Array.prototype.fill() along with the ** operator instead of using a
loop fixes the issue.

PR-URL: #21658
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax
Copy link
Member

The promised follow-up is in #21794 :)

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

Successfully merging this pull request may close these issues.

6 participants