Oddball backport: MaybeStackBuffer + 7462 (Upgrade cpplint) + memory leak fix from 7374 #7666
Conversation
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
addaleax
added
c++
nodejs-github-bot
added
the
build
Contributor
|
@addaleax is this ready to land yet? what's the deal with the [SQUSH] commits? Looks like you may need to do a rebase |
MylesBorins
force-pushed
the
v4.x-staging
branch
from
808fb1f to
9079d87
Compare
Member
Author
|
The
I’d definitely like @bnoordhuis to give this a quick look before landing. |
addaleax
force-pushed
the
backport-MaybeStackBuffer
branch
from
2ae3c2e to
fe6fedf
Compare
Member
Author
|
Also, rebased. CI: https://ci.nodejs.org/job/node-test-commit/4069/ |
Merged
Add a basic regression test that checks if the map for IncomingMessage and OutgoingMessage objects is stable over time. The test is not exhaustive in that it doesn't try to establish whether the transition path is the same on every request, it just checks that objects in their final states have the same map. To be investigated why the first (and only the first) ServerRequest object ends up with a deprecated map, regardless of the number of iterations. PR-URL: nodejs#7003 Refs: nodejs#6294 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fixes: nodejs#6925 PR-URL: nodejs#7270 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Refs: nodejs#6655 PR-URL: nodejs#6694 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ingvar Stepanyan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> PR-URL: nodejs#7407
PR-URL: nodejs#7320 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
PR-URL: nodejs#7364 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Clarify how the encoding option interacts with the data type of child process stdout and stderr. Fixes: nodejs#6666 PR-URL: nodejs#7361 Reviewed-By: Colin Ihrig <[email protected]>
Fix typo in example PR-URL: nodejs#7411 Reviewed-By: Brian White <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]>
Remove special handling when asserting on a pair of arguments objects. The code being removed will only run if both `expected` and `actual` are arguments objects. Given that situation, the subsequent code for handling everything else works just fine. Tests added to confirm expected behavior. This came about while trying to improve test coverage. The segment of code removed had no test coverage. I was unable to write a test that would both exercise the code and fail if the code was removed. Further examination indicated that this was because the special handling was not needed. PR-URL: nodejs#7413 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#7466 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#7467 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
The example changed by this commit uses ['ignore'] where 'ignore' is intended. Fixes: nodejs#7269 PR-URL: nodejs#7540 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: João Reis <[email protected]> PR-URL: nodejs#7567
PR-URL: nodejs#7528 Reviewed-By: Anna Henningsen <[email protected]>
Increase socket timeout so that there is enough time to reliably run the test on FreeBSD. Fixes: nodejs#7516 PR-URL: nodejs#7555 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Added clarification about the linter execution. PR-URL: nodejs#7534 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Add benchmark to "Who to CC". Also, alphabetized the only non-alphabetized subsystem. PR-URL: nodejs#7604 Reviewed-By: Ben Noordhuis <[email protected]>
The dns.resolve documentation stated that an array of IP addresses would be returned in the callback. This is true for everything other than the SOA record which returns an object. This fixes that documentation. Fixes: nodejs#6506 PR-URL: nodejs#7532 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
`oldDirs` is assigned but never used. Remove it. (This was missed by the linter in previous versions of ESLint but is flagged by the current version. Updating the linter is contingent on this change or some similar remedy landing.) PR-URL: nodejs#7594 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
Obsoleted by the recent cpplint upgrade. PR-URL: nodejs#7462 Reviewed-By: Trevor Norris <[email protected]>
Pointed out by Coverity. Introduced in commit 05d30d5 from July 2015 ("fs: implemented WriteStream#writev"). WriteBuffers() leaked memory in the synchronous uv_fs_write() error path when trying to write > 1024 buffers. PR-URL: nodejs#7374 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
addaleax
force-pushed
the
backport-MaybeStackBuffer
branch
from
fe6fedf to
3b63592
Compare
Member
|
LGTM, thanks. CI again, there were some spurious failures in the last run: https://ci.nodejs.org/job/node-test-pull-request/3263/ |
Member
|
The fix-up to src/atomic-polyfill.h can be dropped if #7675 lands first. |
Member
MylesBorins
force-pushed
the
v4.x-staging
branch
from
fcdb93d to
27222f4
Compare
Contributor
|
landed in f886b01...6aa1511 Things look kinda gnarly in here due to rebasing out the offending test that was causing failures here. Will be digging in a researching that independently. edit: also #7675 landed first so the fix-up to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backported all of these together because c50e192 (the missing commit from #7374 in the backport at #7663) depended on one of the cpplint changes, hope that’s okay for you.
/cc @thealphanerd @bnoordhuis