tracing: Fix some race conditions #22812

ofrobots · 2018-09-12T00:28:09Z

This PR contains two commits that fix some race conditions that were causing flakiness on #21038. There may yet be more races, but this is what I got right now.

One of the commits is an upstream V8 fix. I didn't bother requesting an upstream back-port as it is not particular useful for Chrome and better to float here.

Checklist

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

Original commit message: [tracing] do not add traces when disabled nodejs#21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#55809} Refs: v8/v8@2363cdf

Fixes: nodejs#21038

nodejs-github-bot · 2018-09-12T00:28:11Z

@ofrobots build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/858/pipeline

refack · 2018-09-12T00:33:57Z

src/tracing/node_trace_buffer.cc

@@ -59,7 +59,13 @@ void InternalTraceBuffer::Flush(bool blocking) {
      for (size_t i = 0; i < total_chunks_; ++i) {
        auto& chunk = chunks_[i];
        for (size_t j = 0; j < chunk->size(); ++j) {
-          agent_->AppendTraceEvent(chunk->GetEventAt(j));
+          TraceObject* trace_event = chunk->GetEventAt(j);


Why not auto?

We tend to avoid auto, except for cases where the type is hard to spell out (e.g. STL iterator types/dependent on templating), or long and easy to infer (e.g. auto foo = new VeryLongClassName();). Using auto means spending more time writing the code, but more time reading it, and Node.js code is typically read a lot more than it is written.

IMHO it's time for that to change (ES.11).
Beside using the CCG to not bikeshed, for me as a reader TraceObject* is as opaque as auto, that why we have IDEs.

jasnell

I think this actually fixes a problem I've been seeing in some of the other dev work I've been doing. LGTM!

ofrobots · 2018-09-12T17:01:13Z

Upstream change got flagged by UBSAN. I need to investigate those before this lands.

ofrobots · 2018-09-13T17:14:25Z

UBSAN failures were unrelated to my change 🎉.

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

ofrobots · 2018-09-13T20:37:53Z

CI looks green. I will land this later today.

ofrobots · 2018-09-14T01:12:47Z

Landed as 5cccc55...90c9368.

Original commit message: [tracing] do not add traces when disabled #21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#55809} Refs: v8/v8@2363cdf PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>

PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>

Original commit message: [tracing] do not add traces when disabled #21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#55809} Refs: v8/v8@2363cdf PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>

PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>

Original commit message: [tracing] do not add traces when disabled #21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#55809} Refs: v8/v8@2363cdf PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>

PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>

Original commit message: [tracing] do not add traces when disabled #21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#55809} Refs: v8/v8@2363cdf PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>

PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>

Unreliability for test-trace-events-fs-sync is believed to have been fixed. Remove flaky designation. Ref: nodejs#22812 Ref: nodejs#21038 (comment)

Unreliability for test-trace-events-fs-sync is believed to have been fixed. Remove flaky designation. Ref: nodejs#22812 Ref: nodejs#21038 (comment) PR-URL: nodejs#22856 Refs: nodejs#22812 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Unreliability for test-trace-events-fs-sync is believed to have been fixed. Remove flaky designation. Ref: #22812 Ref: #21038 (comment) PR-URL: #22856 Refs: #22812 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

ofrobots added 2 commits September 11, 2018 15:40

tracing: avoid flusing uninitialized traces

ac56580

Fixes: nodejs#21038

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Sep 12, 2018

refack approved these changes Sep 12, 2018

View reviewed changes

refack reviewed Sep 12, 2018

View reviewed changes

jasnell approved these changes Sep 12, 2018

View reviewed changes

addaleax approved these changes Sep 12, 2018

View reviewed changes

eugeneo approved these changes Sep 12, 2018

View reviewed changes

ofrobots added the wip Issues and PRs that are still a work in progress. label Sep 12, 2018

ofrobots removed the wip Issues and PRs that are still a work in progress. label Sep 13, 2018

ofrobots closed this Sep 14, 2018

ofrobots deleted the fix/21308 branch September 14, 2018 01:13

ofrobots mentioned this pull request Sep 14, 2018

Flaky parallel/test-trace-events-fs-sync #21038

Closed

Trott mentioned this pull request Sep 14, 2018

test: remove flaky designation for test #22856

Closed

2 tasks

targos mentioned this pull request Sep 18, 2018

Release proposal: v10.11.0 #22932

Merged

Trott added a commit to Trott/io.js that referenced this pull request Oct 6, 2018

test: remove flaky designation for test

0b924f4

Unreliability for test-trace-events-fs-sync is believed to have been fixed. Remove flaky designation. Ref: nodejs#22812 Ref: nodejs#21038 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: Fix some race conditions #22812

tracing: Fix some race conditions #22812

ofrobots commented Sep 12, 2018

nodejs-github-bot commented Sep 12, 2018

refack Sep 12, 2018

addaleax Sep 12, 2018 •

edited

Loading

refack Sep 14, 2018

jasnell left a comment

ofrobots commented Sep 12, 2018

ofrobots commented Sep 13, 2018

ofrobots commented Sep 13, 2018

ofrobots commented Sep 14, 2018 •

edited

Loading

tracing: Fix some race conditions #22812

tracing: Fix some race conditions #22812

Conversation

ofrobots commented Sep 12, 2018

Checklist

nodejs-github-bot commented Sep 12, 2018

refack Sep 12, 2018

Choose a reason for hiding this comment

addaleax Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

refack Sep 14, 2018

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

ofrobots commented Sep 12, 2018

ofrobots commented Sep 13, 2018

ofrobots commented Sep 13, 2018

ofrobots commented Sep 14, 2018 • edited Loading

addaleax Sep 12, 2018 •

edited

Loading

ofrobots commented Sep 14, 2018 •

edited

Loading