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

tools: fix prof polyfill readline #18641

Closed
wants to merge 6 commits into from

Conversation

killagu
Copy link
Contributor

@killagu killagu commented Feb 8, 2018

node --prof foo.js may not print the full profile log file,
the last line will broken like tick, and not more blank line.
readlinewill stuck in infinite loop, add bytes < buf.length
will fix it.

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)

@bnoordhuis

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Feb 8, 2018
@killagu
Copy link
Contributor Author

killagu commented Feb 8, 2018

node v9.4.0 and v9.5.0 may not print the full profile log file when use the ctrl-c close the process.


const LOG_FILE = path.join(tmpdir.path, 'tick-processor.log');
const RETRY_TIMEOUT = 150;
const BORKEN_PART = 'tick,0x100840650';
Copy link
Contributor

@mmarchini mmarchini Feb 8, 2018

Choose a reason for hiding this comment

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

typo s/BORKEN_PART/BROKEN_PART/

Also, could we just use const BROKEN_PART = 'tick,'/ instead? "0x100840650" looks like a magic number here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes,it's better.

@killagu
Copy link
Contributor Author

killagu commented Feb 9, 2018

@mmarchini the magic number have been removed.

@Fishrock123
Copy link
Contributor

common.isAIX ||
common.isLinuxPPCBE ||
common.isFreeBSD)
common.skip('C++ symbols are not mapped for this os.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether the linter (make lint) will complain or not, but this is difficult to read. Please use braces here.

@killagu killagu force-pushed the fix-prof-polyfill branch 2 times, most recently from 479f168 to fd9baf5 Compare February 11, 2018 02:33
@@ -93,7 +93,7 @@ function readline() {
}
const bytes = fs.readSync(fd, buf, 0, buf.length);
line += dec.write(buf.slice(0, bytes));
if (line.length === 0) {
if (line.length === 0 || bytes < buf.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

emit a warning while broken?

`node --prof foo.js` may not print the full profile log file,
the last line will broken like `tick,` and not more blank line.
`readline`will stuck in infinite loop, add `bytes === 0`
will fix it.
Copy link
Contributor

@XadillaX XadillaX left a comment

Choose a reason for hiding this comment

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

LGTM if CI is OK.

@XadillaX
Copy link
Contributor

@XadillaX
Copy link
Contributor

@@ -802,3 +802,8 @@ exports.hijackStdout = hijackStdWritable.bind(null, 'stdout');
exports.hijackStderr = hijackStdWritable.bind(null, 'stderr');
exports.restoreStdout = restoreWritable.bind(null, 'stdout');
exports.restoreStderr = restoreWritable.bind(null, 'stderr');
exports.isSymbolAvailable = exports.isWindows ||
exports.isSunOS ||
Copy link
Contributor

@XadillaX XadillaX Feb 11, 2018

Choose a reason for hiding this comment

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

nit: align with exports above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XadillaX as you wish.

@@ -243,6 +243,11 @@ Platform check for Windows.

Platform check for Windows 32-bit on Windows 64-bit.

### isSymbolAvailable
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a more descriptive name than this please?

Copy link
Contributor Author

@killagu killagu Feb 12, 2018

Choose a reason for hiding this comment

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

my bad,it should be isCPPSymbolsNotMapped. @cjihrig

### isSymbolAvailable
* [&lt;Boolean>]

Platform check for if symbol available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, can you provide a more in depth description.

@killagu
Copy link
Contributor Author

killagu commented Feb 12, 2018

@cjihrig the property name,and readme have been changed to more descriptive.

* [&lt;Boolean>]

Platform check for if symbol available.
Platform check for c++ symbols are mapped or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

c++ => C++

Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

tmpdir.refresh();

if (!common.enoughTestCpu)
common.skip('test is CPU-intensive');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: would you be so kind and use a capital letter as first character? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the node test,the first character in skip message is always lower case.

common.skip('test is CPU-intensive');

if (common.isCPPSymbolsNotMapped) {
common.skip('C++ symbols are not mapped for this os.');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Operating system should be upper cased as in OS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the advice.It should be upper cased.


//This test will produce a broken profile log.
//ensure prof-polyfill not stuck in infinite loop
//and success process
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please use a space at the beginning of each line in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the advice.It should have a space at the beginning of each line.

this.ts = Date.now();
setImmediate(function() { new f(); });
};
f();`;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we have the unwritten rule not to use multi line template strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the advice.I will split it to two lines.

'--prof-process', LOG_FILE
]);
assert(WARN_REG_EXP.test(child.stderr.toString()));
assert.strictEqual(child.status, 0, 'broken file should success too');
Copy link
Member

Choose a reason for hiding this comment

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

I personally would like to stick to the default message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default message will be removed.

@@ -96,6 +96,13 @@ function readline() {
if (line.length === 0) {
return '';
}
if (bytes === 0) {
process.emitWarning('profile file is broken', {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: you might just add the filename of the broken file? And please use a capital letter as first character.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the advice.Add the filename is better.

fix warn message
fix comment style
@killagu
Copy link
Contributor Author

killagu commented Feb 17, 2018

@BridgeAR Thank you for the advice.I have update the code.Please review it.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018
@BridgeAR
Copy link
Member

mmarchini pushed a commit that referenced this pull request Feb 17, 2018
`node --prof foo.js` may not print the full profile log file, leaving
the last line broken (for example `tick,`. When that happens, `readline`
will be stuck in an infinite loop. This patch fixes it.

Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code
on tick-processor tests.

PR-URL: #18641
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@mmarchini
Copy link
Contributor

Landed in 38f04d4 😄

@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@killagu
Copy link
Contributor Author

killagu commented Feb 21, 2018

@MylesBorins It should be backported to v9.x-staging,and I will raise a backport PR.

killagu added a commit to killagu/node that referenced this pull request Feb 21, 2018
`node --prof foo.js` may not print the full profile log file, leaving
the last line broken (for example `tick,`. When that happens, `readline`
will be stuck in an infinite loop. This patch fixes it.

Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code
on tick-processor tests.

PR-URL: nodejs#18641
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@killagu
Copy link
Contributor Author

killagu commented Feb 21, 2018

@MylesBorins I have raised a PR to backport it.

BTW I have no permission for run the CI. If you can help me trigger the CI, you are so kind.

killagu added a commit to killagu/node that referenced this pull request Feb 22, 2018
`node --prof foo.js` may not print the full profile log file, leaving
the last line broken (for example `tick,`. When that happens, `readline`
will be stuck in an infinite loop. This patch fixes it.

Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code
on tick-processor tests.

PR-URL: nodejs#18641
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
`node --prof foo.js` may not print the full profile log file, leaving
the last line broken (for example `tick,`. When that happens, `readline`
will be stuck in an infinite loop. This patch fixes it.

Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code
on tick-processor tests.

Backport-PR-URL: #18901
PR-URL: #18641
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
`node --prof foo.js` may not print the full profile log file, leaving
the last line broken (for example `tick,`. When that happens, `readline`
will be stuck in an infinite loop. This patch fixes it.

Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code
on tick-processor tests.

Backport-PR-URL: #18901
PR-URL: #18641
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax addaleax mentioned this pull request Feb 27, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
`node --prof foo.js` may not print the full profile log file, leaving
the last line broken (for example `tick,`. When that happens, `readline`
will be stuck in an infinite loop. This patch fixes it.

Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code
on tick-processor tests.

PR-URL: nodejs#18641
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
`node --prof foo.js` may not print the full profile log file, leaving
the last line broken (for example `tick,`. When that happens, `readline`
will be stuck in an infinite loop. This patch fixes it.

Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code
on tick-processor tests.

Backport-PR-URL: #18901
PR-URL: #18641
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
`node --prof foo.js` may not print the full profile log file, leaving
the last line broken (for example `tick,`. When that happens, `readline`
will be stuck in an infinite loop. This patch fixes it.

Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code
on tick-processor tests.

Backport-PR-URL: #18901
PR-URL: #18641
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants