src: move v8::HandleScope call to Emit #20045

ryzokuken · 2018-04-15T10:02:45Z

Move v8::HandleScope call to Emit removing it from previous locations
where it was added to avoid crashing (constructor and destructor of
AsyncWrap) for a more general and fool-proof solution.

Ref: #19972 (comment)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

cc @addaleax @hashseed

addaleax · 2018-04-15T15:05:17Z

CI: https://ci.nodejs.org/job/node-test-commit/17773/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/156

(Not sure if the benchmark CI works, had to specify async_hooks by manually editing the build parameter HTML form…)

BridgeAR · 2018-04-15T20:29:36Z

@addaleax are you certain you ran the correct benchmarks? I think you wanted to run async_hooks, no?

addaleax · 2018-04-15T20:33:10Z

@BridgeAR Yeah, that benchmark run missed its target. Maybe I screwed up editing the HTML.

Anyway, when trying to enter async_hooks in the rebuild interface I get a server error, claiming that as an invalid value. :(

@nodejs/build Could you turn category for the microbenchmark tasks into a text field, or update the list of categories?

ryzokuken · 2018-04-17T07:48:47Z

@addaleax would you like to try again with the benchmark CI?

ryzokuken · 2018-04-17T16:44:09Z

Rerunning normal CI.

ryzokuken · 2018-04-17T16:44:42Z

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

Remove redundant calls to v8::HandleScope in the contructor and destructor for the AsyncScope class Refs: nodejs#19972 (comment) Refs: nodejs#20045

ryzokuken · 2018-04-17T18:24:26Z

I couldn't spot the failing test in https://ci.nodejs.org/job/node-test-commit-linux/18087/nodes=fedora-last-latest-x64/console

ryzokuken · 2018-04-17T18:27:43Z

Okay, we have 13:00:08 not ok 191 parallel/test-child-process-exec-timeout.

richardlau · 2018-04-17T18:47:31Z

I couldn't spot the failing test in https://ci.nodejs.org/job/node-test-commit-linux/18087/nodes=fedora-last-latest-x64/console

Look in the full console log:
https://ci.nodejs.org/job/node-test-commit-linux/18087/nodes=fedora-last-latest-x64/consoleFull

richardlau · 2018-04-17T18:49:39Z

not ok 191 parallel/test-child-process-exec-timeout
  ---
  duration_ms: 0.330
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:76
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 'SIGSEGV'
    + 'SIGTERM'
        at cp.exec.common.mustCall (/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora-last-latest-x64/test/parallel/test-child-process-exec-timeout.js:34:12)
        at /home/iojs/build/workspace/node-test-commit-linux/nodes/fedora-last-latest-x64/test/common/index.js:474:15
        at ChildProcess.exithandler (child_process.js:289:5)
        at ChildProcess.emit (events.js:182:13)
        at maybeClose (internal/child_process.js:944:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:233:5)
  ...

ryzokuken · 2018-04-18T20:29:39Z

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

(To make sure it wasn't just a flaky test)

ryzokuken · 2018-04-18T21:25:14Z

Again, a total random failure:

not ok 495 parallel/test-cluster-setup-master
  ---
  duration_ms: 120.220
  severity: fail
  exitcode: -15
  stack: |-
    timeout
  ...

Somebody help?

ryzokuken · 2018-04-18T21:33:58Z

Honestly, the osx log doesn't even specify what went wrong, I think (https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1010/17933/consoleFull)

Move v8::HandleScope call to Emit removing it from previous locations where it was added to avoid crashing (constructor and destructor of AsyncWrap) for a more general and fool-proof solution. Ref: nodejs#19972 (comment)

ryzokuken · 2018-04-19T04:05:25Z

Restarting CI after rebasing on top of current master.

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

ryzokuken · 2018-04-20T03:27:13Z

Restarting after OSX was fixed in #20139.

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

ryzokuken · 2018-04-20T04:27:13Z

@Trott OSX passes for this one! 🎉

Another random failure? damn. BSD never failed on this one. Lemme rebuild.

ryzokuken · 2018-04-20T04:29:08Z

23:55:31 not ok 2150 sequential/test-debugger-debug-brk
23:55:31   ---
23:55:31   duration_ms: 0.540
23:55:31   severity: fail
23:55:31   exitcode: 1
23:55:31   stack: |-
23:55:31     assert.js:77
23:55:31       throw new AssertionError(obj);
23:55:31       ^
23:55:31     
23:55:31     AssertionError [ERR_ASSERTION]: '/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/out/Release/node --inspect --debug-brk /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/fixtures/empty.js' should not quit
23:55:31         at ChildProcess.fail (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/sequential/test-debugger-debug-brk.js:19:29)
23:55:31         at ChildProcess.emit (events.js:182:13)
23:55:31         at Process.ChildProcess._handle.onexit (internal/child_process.js:222:12)

Completely unrelated, rebuilding.

Trott · 2018-04-20T04:49:04Z

Re-running just FreeBSD: https://ci.nodejs.org/job/node-test-commit-freebsd/17164/

ryzokuken · 2018-04-20T05:23:55Z

@Trott it passed! Should I make an issue for a flaky test?

BTW, I'm landing this. It has been open for 5 days and has been approved by a ton of people.

ryzokuken · 2018-04-20T05:27:37Z

Landed in e32eddc

Move v8::HandleScope call to Emit removing it from previous locations where it was added to avoid crashing (constructor and destructor of AsyncWrap) for a more general and fool-proof solution. Ref: #19972 (comment) PR-URL: #20045 Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Trott · 2018-04-20T16:17:53Z

Should I make an issue for a flaky test?

If an issue doesn't exist, that would be helpful. We should always do that, but I know I don't because...so many flaky tests...only so many hours to spend in the Node.js issue tracker a day, you know? :-D

In that particular case, flakiness might be because some other process is taking the predefined port in the test? Not sure, just a guess.

Refs: nodejs#19972 (comment) Refs: nodejs#20045

Move v8::HandleScope call to Emit removing it from previous locations where it was added to avoid crashing (constructor and destructor of AsyncWrap) for a more general and fool-proof solution. Ref: #19972 (comment) PR-URL: #20045 Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 15, 2018

hashseed approved these changes Apr 15, 2018

View reviewed changes

cjihrig approved these changes Apr 15, 2018

View reviewed changes

addaleax approved these changes Apr 15, 2018

View reviewed changes

TimothyGu approved these changes Apr 15, 2018

View reviewed changes

jasnell approved these changes Apr 16, 2018

View reviewed changes

danbev approved these changes Apr 17, 2018

View reviewed changes

ryzokuken added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 17, 2018

ryzokuken added a commit to ryzokuken/node that referenced this pull request Apr 17, 2018

src: remove redundant calls to v8::HandleScope

4a1e88f

Remove redundant calls to v8::HandleScope in the contructor and destructor for the AsyncScope class Refs: nodejs#19972 (comment) Refs: nodejs#20045

ryzokuken mentioned this pull request Apr 17, 2018

src: remove redundant calls to v8::HandleScope #20110

Closed

2 tasks

ryzokuken force-pushed the bugfix/v8-handlescope branch from 171f3d8 to b9b7b01 Compare April 19, 2018 04:04

src: move v8::HandleScope call to Emit

b9b7b01

Move v8::HandleScope call to Emit removing it from previous locations where it was added to avoid crashing (constructor and destructor of AsyncWrap) for a more general and fool-proof solution. Ref: nodejs#19972 (comment)

ryzokuken mentioned this pull request Apr 19, 2018

test-child-process-exec-kill-throws fails intermittently on multiple platforms #20139

Closed

ryzokuken closed this Apr 20, 2018

ryzokuken added a commit to ryzokuken/node that referenced this pull request Apr 20, 2018

src: remove redundant calls to HandleScope

538ef2f

Refs: nodejs#19972 (comment) Refs: nodejs#20045

This was referenced Apr 20, 2018

src: remove redundant calls to HandleScope #20177

Closed

flaky test-debugger-debug-brk #18435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: move v8::HandleScope call to Emit #20045

src: move v8::HandleScope call to Emit #20045

ryzokuken commented Apr 15, 2018

addaleax commented Apr 15, 2018 •

edited

Loading

BridgeAR commented Apr 15, 2018

addaleax commented Apr 15, 2018

ryzokuken commented Apr 17, 2018

ryzokuken commented Apr 17, 2018

ryzokuken commented Apr 17, 2018

ryzokuken commented Apr 17, 2018

ryzokuken commented Apr 17, 2018

richardlau commented Apr 17, 2018

richardlau commented Apr 17, 2018

ryzokuken commented Apr 18, 2018 •

edited

Loading

ryzokuken commented Apr 18, 2018 •

edited

Loading

ryzokuken commented Apr 18, 2018

ryzokuken commented Apr 19, 2018

ryzokuken commented Apr 20, 2018

ryzokuken commented Apr 20, 2018

ryzokuken commented Apr 20, 2018

Trott commented Apr 20, 2018

ryzokuken commented Apr 20, 2018

ryzokuken commented Apr 20, 2018

Trott commented Apr 20, 2018

src: move v8::HandleScope call to Emit #20045

src: move v8::HandleScope call to Emit #20045

Conversation

ryzokuken commented Apr 15, 2018

Checklist

addaleax commented Apr 15, 2018 • edited Loading

BridgeAR commented Apr 15, 2018

addaleax commented Apr 15, 2018

ryzokuken commented Apr 17, 2018

ryzokuken commented Apr 17, 2018

ryzokuken commented Apr 17, 2018

ryzokuken commented Apr 17, 2018

ryzokuken commented Apr 17, 2018

richardlau commented Apr 17, 2018

richardlau commented Apr 17, 2018

ryzokuken commented Apr 18, 2018 • edited Loading

ryzokuken commented Apr 18, 2018 • edited Loading

ryzokuken commented Apr 18, 2018

ryzokuken commented Apr 19, 2018

ryzokuken commented Apr 20, 2018

ryzokuken commented Apr 20, 2018

ryzokuken commented Apr 20, 2018

Trott commented Apr 20, 2018

ryzokuken commented Apr 20, 2018

ryzokuken commented Apr 20, 2018

Trott commented Apr 20, 2018

addaleax commented Apr 15, 2018 •

edited

Loading

ryzokuken commented Apr 18, 2018 •

edited

Loading

ryzokuken commented Apr 18, 2018 •

edited

Loading