tools: fix prof polyfill readline #18641

killagu · 2018-02-08T13:04:11Z

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

killagu · 2018-02-08T13:09:50Z

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

mmarchini · 2018-02-08T23:16:11Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

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


typo s/BORKEN_PART/BROKEN_PART/

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

yes,it's better.

killagu · 2018-02-09T01:11:33Z

@mmarchini the magic number have been removed.

Fishrock123 · 2018-02-10T17:08:00Z

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

Fishrock123 · 2018-02-10T17:08:40Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

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


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

XadillaX · 2018-02-11T03:02:31Z

lib/internal/v8_prof_polyfill.js

@@ -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) {


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.

XadillaX

LGTM if CI is OK.

XadillaX · 2018-02-11T03:53:14Z

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

XadillaX · 2018-02-11T04:06:30Z

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

XadillaX · 2018-02-11T09:22:02Z

test/common/index.js

@@ -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 ||


nit: align with exports above.

@XadillaX as you wish.

cjihrig · 2018-02-11T20:03:26Z

test/common/README.md

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

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

+### isSymbolAvailable


Can we use a more descriptive name than this please?

my bad,it should be isCPPSymbolsNotMapped. @cjihrig

cjihrig · 2018-02-11T20:04:13Z

test/common/README.md

+### isSymbolAvailable
+* [&lt;Boolean>]
+
+Platform check for if symbol available.


Similarly, can you provide a more in depth description.

killagu · 2018-02-12T01:16:40Z

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

XadillaX · 2018-02-12T02:14:48Z

test/common/README.md

 * [&lt;Boolean>]

-Platform check for if symbol available.
+Platform check for c++ symbols are mapped or not.


c++ => C++

mmarchini

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

BridgeAR · 2018-02-16T21:28:34Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

+tmpdir.refresh();
+
+if (!common.enoughTestCpu)
+  common.skip('test is CPU-intensive');


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

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

BridgeAR · 2018-02-16T21:29:11Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

+  common.skip('test is CPU-intensive');
+
+if (common.isCPPSymbolsNotMapped) {
+  common.skip('C++ symbols are not mapped for this os.');


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

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

BridgeAR · 2018-02-16T21:29:54Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

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


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

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

BridgeAR · 2018-02-16T21:46:07Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

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


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

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

BridgeAR · 2018-02-16T21:46:43Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

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


I personally would like to stick to the default message.

The default message will be removed.

BridgeAR · 2018-02-16T21:50:13Z

lib/internal/v8_prof_polyfill.js

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


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

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

fix warn message fix comment style

killagu · 2018-02-17T00:49:39Z

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

BridgeAR · 2018-02-17T01:19:32Z

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

`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 <i@2333.moe> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

mmarchini · 2018-02-17T12:09:25Z

Landed in 38f04d4 😄

MylesBorins · 2018-02-21T15:11:13Z

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 · 2018-02-21T15:33:38Z

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

`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 <i@2333.moe> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

killagu · 2018-02-21T16:26:22Z

@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.

`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 <i@2333.moe> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

`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 <i@2333.moe> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

`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 <i@2333.moe> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

`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 <i@2333.moe> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Feb 8, 2018

mmarchini reviewed Feb 8, 2018

View reviewed changes

killagu force-pushed the fix-prof-polyfill branch from 7b4e27c to bfaee12 Compare February 9, 2018 01:10

Fishrock123 reviewed Feb 10, 2018

View reviewed changes

killagu force-pushed the fix-prof-polyfill branch 2 times, most recently from 479f168 to fd9baf5 Compare February 11, 2018 02:33

XadillaX reviewed Feb 11, 2018

View reviewed changes

killagu force-pushed the fix-prof-polyfill branch from 9b54b62 to 41f92a4 Compare February 11, 2018 03:47

tools: fix prof polyfill readline

33f4a51

`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.

killagu force-pushed the fix-prof-polyfill branch from 41f92a4 to 33f4a51 Compare February 11, 2018 03:50

XadillaX approved these changes Feb 11, 2018

View reviewed changes

test: add common.isSymbolAvailable

45730d5

XadillaX reviewed Feb 11, 2018

View reviewed changes

fix: fix code style

2610183

cjihrig reviewed Feb 11, 2018

View reviewed changes

mv common.isSymbolAvailable -> common.isCPPSymbolsNotMapped

05b0391

XadillaX reviewed Feb 12, 2018

View reviewed changes

fix c++ -> C++

3684bc0

mmarchini approved these changes Feb 13, 2018

View reviewed changes

BridgeAR reviewed Feb 16, 2018

View reviewed changes

fix code style

a43951e

fix warn message fix comment style

BridgeAR approved these changes Feb 17, 2018

View reviewed changes

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

mmarchini closed this Feb 17, 2018

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

killagu mentioned this pull request Feb 21, 2018

[v9.x backport] tools, test: fix prof polyfill readline #18900

Closed

4 tasks

killagu mentioned this pull request Feb 21, 2018

[v9.x backport] tools, test: fix prof polyfill readline #18901

Closed

2 tasks

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Feb 26, 2018

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

MylesBorins added the land-on-v8.x label Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

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

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

		### isSymbolAvailable

tools: fix prof polyfill readline #18641

tools: fix prof polyfill readline #18641

Conversation

killagu commented Feb 8, 2018 • edited Loading

Checklist

Affected core subsystem(s)

killagu commented Feb 8, 2018

mmarchini Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killagu commented Feb 9, 2018

Fishrock123 commented Feb 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX left a comment • edited Loading

Choose a reason for hiding this comment

XadillaX commented Feb 11, 2018

XadillaX commented Feb 11, 2018

XadillaX Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killagu Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killagu commented Feb 12, 2018

Choose a reason for hiding this comment

mmarchini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killagu commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

mmarchini commented Feb 17, 2018

MylesBorins commented Feb 21, 2018

killagu commented Feb 21, 2018

killagu commented Feb 21, 2018

killagu commented Feb 8, 2018 •

edited

Loading

mmarchini Feb 8, 2018 •

edited

Loading

XadillaX left a comment •

edited

Loading

XadillaX Feb 11, 2018 •

edited

Loading

killagu Feb 12, 2018 •

edited

Loading