Skip to content

Commit

Permalink
tools, test: fix prof polyfill readline
Browse files Browse the repository at this point in the history
`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]>
  • Loading branch information
killagu authored and rvagg committed Aug 16, 2018
1 parent 1d90700 commit c4716dc
Show file tree
Hide file tree
Showing 7 changed files with 86 additions and 15 deletions.
7 changes: 7 additions & 0 deletions lib/internal/v8_prof_polyfill.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ function readline() {
if (line.length === 0) {
return '';
}
if (bytes === 0) {
process.emitWarning(`Profile file ${logFile} is broken`, {
code: 'BROKEN_PROFILE_FILE',
detail: `${JSON.stringify(line)} at the file end is broken`
});
return '';
}
}
}

Expand Down
5 changes: 5 additions & 0 deletions test/common/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,11 @@ Platform check for Windows.

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

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

Platform check for C++ symbols are mapped or not.

### leakedGlobals()
* return [&lt;Array>]

Expand Down
6 changes: 6 additions & 0 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -833,3 +833,9 @@ exports.firstInvalidFD = function firstInvalidFD() {
} catch (e) {}
return fd;
};

exports.isCPPSymbolsNotMapped = exports.isWindows ||
exports.isSunOS ||
exports.isAIX ||
exports.isLinuxPPCBE ||
exports.isFreeBSD;
7 changes: 2 additions & 5 deletions test/tick-processor/test-tick-processor-builtin.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,9 @@ const common = require('../common');
if (!common.enoughTestCpu)
common.skip('test is CPU-intensive');

if (common.isWindows ||
common.isSunOS ||
common.isAIX ||
common.isLinuxPPCBE ||
common.isFreeBSD)
if (common.isCPPSymbolsNotMapped) {
common.skip('C++ symbols are not mapped for this os.');
}

const base = require('./tick-processor-base.js');

Expand Down
7 changes: 2 additions & 5 deletions test/tick-processor/test-tick-processor-cpp-core.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,9 @@ const common = require('../common');
if (!common.enoughTestCpu)
common.skip('test is CPU-intensive');

if (common.isWindows ||
common.isSunOS ||
common.isAIX ||
common.isLinuxPPCBE ||
common.isFreeBSD)
if (common.isCPPSymbolsNotMapped) {
common.skip('C++ symbols are not mapped for this os.');
}

const base = require('./tick-processor-base.js');

Expand Down
62 changes: 62 additions & 0 deletions test/tick-processor/test-tick-processor-polyfill-brokenfile.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
'use strict';
const common = require('../common');
const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

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

if (common.isCPPSymbolsNotMapped) {
common.skip('C++ symbols are not mapped for this OS.');
}

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


const assert = require('assert');
const cp = require('child_process');
const path = require('path');
const fs = require('fs');

const LOG_FILE = path.join(tmpdir.path, 'tick-processor.log');
const RETRY_TIMEOUT = 150;
const BROKEN_PART = 'tick,';
const WARN_REG_EXP = /\(node:\d+\) \[BROKEN_PROFILE_FILE] Warning: Profile file .* is broken/;
const WARN_DETAIL_REG_EXP = /".*tick," at the file end is broken/;

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

const proc = cp.spawn(process.execPath, [
'--no_logfile_per_isolate',
'--logfile=-',
'--prof',
'-pe', code
], {
stdio: ['ignore', 'pipe', 'inherit']
});

let ticks = '';
proc.stdout.on('data', (chunk) => ticks += chunk);


function runPolyfill(content) {
proc.kill();
content += BROKEN_PART;
fs.writeFileSync(LOG_FILE, content);
const child = cp.spawnSync(
`${process.execPath}`,
[
'--prof-process', LOG_FILE
]);
assert(WARN_REG_EXP.test(child.stderr.toString()));
assert(WARN_DETAIL_REG_EXP.test(child.stderr.toString()));
assert.strictEqual(child.status, 0);
}

setTimeout(() => runPolyfill(ticks), RETRY_TIMEOUT);
7 changes: 2 additions & 5 deletions test/tick-processor/test-tick-processor-preprocess-flag.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,9 @@ const common = require('../common');
if (!common.enoughTestCpu)
common.skip('test is CPU-intensive');

if (common.isWindows ||
common.isSunOS ||
common.isAIX ||
common.isLinuxPPCBE ||
common.isFreeBSD)
if (common.isCPPSymbolsNotMapped) {
common.skip('C++ symbols are not mapped for this os.');
}

const base = require('./tick-processor-base.js');

Expand Down

0 comments on commit c4716dc

Please sign in to comment.