Revert "src: remove unused function" #18049

jasnell · 2018-01-09T00:44:50Z

This reverts commit d5cef71.

Note: This targets v9.x-staging, not master

This commit should not have landed as it was dependent on a semver-major. This should be fast tracked to unbreak v9.x-staging

Fixes: #18048

ping @MylesBorins @cjihrig

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

The function IsInt64() in src/node_file.cc is no longer used. This commit removes it. PR-URL: nodejs#17671 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Jon Moss <[email protected]>

`validChunk` allowed `undefined` as a chunk in object mode; however, this was redundant, since: - `validChunk()` is only used by `.write()` - If the `validChunk()` check passes for `undefined`, `.write()` calls `writeOrBuffer()` - If `writeOrBuffer()` does not receive a Buffer, it calls `decodeChunk()` - `decodeChunk()` ignores `undefined` because it checks `typeof chunk === 'string'` - After that call, `chunk.length` is accessed, which throws an error if `chunk` is undefined`. This “fixes” a bug in the sense that `state.pendingcb` is no longer incremented for write attempts that fail like this. PR-URL: nodejs#17644 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]>

This is superfluous now that typechecking in `net` and `stream` are aligned. PR-URL: nodejs#17644 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]>

Adds instructions on how to format C++ comments to the C++ style guide, as `cpplint.py` doesn't complain about C-style comments on the code. PR-URL: nodejs#17617 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Remove a pointless adapter frame by fixing up the function's formal parameter count. Before: frame #0: 0x000033257ea446d5 onParserExecute(...) frame #1: 0x000033257ea3b93f <adaptor> frame #2: 0x000033257ea41959 <internal> frame #3: 0x000033257e9840ff <entry> After: frame #0: 0x00000956287446d5 onParserExecute(...) frame #1: 0x0000095628741959 <internal> frame #2: 0x00000956286840ff <entry> PR-URL: nodejs#17693 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

This adds computed properties to readable and writable streams to allow access to the readable buffer, the writable buffer, and flow state without accessing the readable or writable state. These are the only uses of readable and writable state in the docs so adding these work arounds allows them to be removed from the docs. This also updates net, http_client and http_server to use the new methods instead of manipulating readable and writable state directly. See: nodejs#445 PR-URL: nodejs#12855 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

Adds a tests that checks if we can start reading again from a socket backing an incoming http request. PR-URL: nodejs#17654 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

PR-URL: nodejs#17699 Reviewed-By: Yosuke Furukawa <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#17698 Reviewed-By: Yosuke Furukawa <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#17697 Reviewed-By: Yosuke Furukawa <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#17643 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

This should make these function calls a lot more intuitive for people who are more accustomed to Node’s EventEmitter API. PR-URL: nodejs#17701 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#17711 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

Change type annotations for eventName in events API doc from {any} to {string|symbol}. PR-URL: nodejs#17666 Fixes: nodejs#17657 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#17665 Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Change the awkward "Node.js style callback" phrasing to the more informative "error-first callback." PR-URL: nodejs#17638 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

PR-URL: nodejs#17635 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

PR-URL: nodejs#17201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Jon Moss <[email protected]>

Fixes: nodejs#17169 PR-URL: nodejs#17646 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

As part of the readableState/writableState mega issue nodejs#445, this removes all of the references to .length on those properties and replaces them with a readableLength and writableLength getter. See: nodejs#445 PR-URL: nodejs#12857 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

This commit adds a test to validate postmortem debugging metadata. When this test runs, it can check for the presence of metadata constants used by tools such as llnode and mdb and report if any have accidentally been removed. PR-URL: nodejs#17685 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Reworded some parts, inter-doc links. PR-URL: nodejs#17702 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

Also works on directories. PR-URL: nodejs#17702 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

PR-URL: nodejs#17702 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

PR-URL: nodejs#17692 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

PR-URL: nodejs#17714 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>

Specify that $VERSION should include the `v` when replacing REPLACEME in documentation. PR-URL: nodejs#17677 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Make the preamble to CONTRIBUTING.md more concise and focused. PR-URL: nodejs#17700 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#17950 Fixes: nodejs#16875 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

5425e0d switched to using the module.exports pattern vs just exports, but left a duplicate export around for OutgoingMessage. PR-URL: nodejs#17982 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

PR-URL: nodejs#17949 Fixes: nodejs#17857 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#17948 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Fixes: nodejs#17370 PR-URL: nodejs#17412 Fixes: nodejs#17370 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

Moved from https://github.com/watilde/remark-preset-lint-node. PR-URL: nodejs#17441 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Before this change, the .PHONY is followed by multiple targets. Now it is multiple .PHONY for each target. PR-URL: nodejs#17964 Refs: nodejs#16968 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#17968 Reviewed-By: Anna Henningsen <[email protected]>

The function should strictly test for the error class and only accept the correct one. Any other error class should fail. PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: nodejs#17955 Refs: nodejs#16317 Reviewed-By: James M Snell <[email protected]>

Matt is not an active collaborator at this time. Moved to the Collaborator Emeriti section. PR-URL: nodejs#17998 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matthew Loring <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Examples in the N-API doc used a mix of nullptr and NULL. We should be consistent and because N-API is a 'C' API I believe using NULL is better. This will avoid any potential confusion as to whether N-API can be used with plain C. PR-URL: nodejs#18008 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

The streams implementation generally ensures that only one write() call is active at a time. `JSStreamWrap` instances still kept queue of write reqeuests in spite of that; refactor it away. Also, fold `isAlive()` into a constant function on the native side. PR-URL: nodejs#17918 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

This reverts commit d5cef71.

jasnell · 2018-01-09T00:45:54Z

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

jasnell · 2018-01-09T00:54:46Z

@MylesBorins ... in addition to this commit, there appears to be some other issues with v9.x-staging: https://ci.nodejs.org/job/node-test-commit-linux/15439/

mcollina

LGTM

MylesBorins · 2018-01-09T08:14:55Z

Fwiw there is no need to land a revert on staging. We can simply rebase out the offending commits

…

On Jan 9, 2018 2:49 AM, "Matteo Collina" ***@***.***> wrote: ***@***.**** approved this pull request. LGTM — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18049 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV15IRcykYSkE85WZlSzQPUd3xAhYks5tIxoWgaJpZM4RXKPA> .

MylesBorins · 2018-01-09T08:20:38Z

Closing as the offending commit has been rebased out.

cjihrig and others added 30 commits January 8, 2018 02:21

test: check socketOnDrain where needPause is false

ad99539

Adds a tests that checks if we can start reading again from a socket backing an incoming http request. PR-URL: nodejs#17654 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

doc: remove extra whitespace in module docs

9586f66

PR-URL: nodejs#17711 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

test: coverage for emitExperimentalWarning

fbe368f

PR-URL: nodejs#17635 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

doc: add countdown module to writing tests guide

92d08d9

PR-URL: nodejs#17201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Jon Moss <[email protected]>

test: update test-http-content-length to use countdown

c626f8a

PR-URL: nodejs#17201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Jon Moss <[email protected]>

doc: improve documentation.md

b83fc9b

Reworded some parts, inter-doc links. PR-URL: nodejs#17702 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

doc: fix fs.existsSync description

07be7fb

Also works on directories. PR-URL: nodejs#17702 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

doc: adjust TTY wording & add inter-doc links

4c3d34e

PR-URL: nodejs#17702 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

doc: not all example code can be run without 1:1

d7cf473

PR-URL: nodejs#17702 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

doc: some fs doc improvements

e70e083

PR-URL: nodejs#17692 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

apapirovski and others added 15 commits January 8, 2018 02:51

test: fix flaky test-pipe-unref

ec9f361

PR-URL: nodejs#17950 Fixes: nodejs#16875 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

test: fix flaky test-http-highwatermark

8b9e478

PR-URL: nodejs#17949 Fixes: nodejs#17857 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

http2: verify that a dependency cycle may exist

a069f44

PR-URL: nodejs#17968 Reviewed-By: Anna Henningsen <[email protected]>

test: add more asserts to test-internal-errors

015c146

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

test: fix flaky test-http-pipeline-flood

7711f00

PR-URL: nodejs#17955 Refs: nodejs#16317 Reviewed-By: James M Snell <[email protected]>

Revert "src: remove unused function"

c11afbf

This reverts commit d5cef71.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. v9.x labels Jan 9, 2018

cjihrig approved these changes Jan 9, 2018

View reviewed changes

jasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 9, 2018

jasnell mentioned this pull request Jan 9, 2018

http2: backport changes to v9.x-staging #18050

Closed

4 tasks

danbev approved these changes Jan 9, 2018

View reviewed changes

targos approved these changes Jan 9, 2018

View reviewed changes

mcollina approved these changes Jan 9, 2018

View reviewed changes

MylesBorins force-pushed the v9.x-staging branch from bd744b1 to f55a5e8 Compare January 9, 2018 08:20

MylesBorins closed this Jan 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "src: remove unused function" #18049

Revert "src: remove unused function" #18049

jasnell commented Jan 9, 2018 •

edited

Loading

jasnell commented Jan 9, 2018

jasnell commented Jan 9, 2018

mcollina left a comment

MylesBorins commented Jan 9, 2018 via email

MylesBorins commented Jan 9, 2018

Revert "src: remove unused function" #18049

Revert "src: remove unused function" #18049

Conversation

jasnell commented Jan 9, 2018 • edited Loading

Checklist

Affected core subsystem(s)

jasnell commented Jan 9, 2018

jasnell commented Jan 9, 2018

mcollina left a comment

Choose a reason for hiding this comment

MylesBorins commented Jan 9, 2018 via email

MylesBorins commented Jan 9, 2018

jasnell commented Jan 9, 2018 •

edited

Loading