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.

Sign up for GitHub

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

Jump to bottom

src: nullcheck on trace controller #25943

Closed
wants to merge 1 commit into from
Closed

src: nullcheck on trace controller #25943

wants to merge 1 commit into from

Conversation

gireeshpunathil
Copy link
Member

Insert a NULLCHECK prior to return. Ideally we do this in the caller,
but the TraceController object is somewhat special as:

  1. It is accessed by most threads
  2. It's life cycle is managed by Agent::Agent
  3. It's getter is invoked through Base Methods (upstream)

Refs: #25814

cc @addaleax

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 5, 2019
@gireeshpunathil gireeshpunathil added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Feb 5, 2019
@gireeshpunathil
Copy link
Member Author

@addaleax - I would like to know your view on this; can you please have a look? thanks!

addaleax
addaleax approved these changes Feb 7, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Can you remove the nullptr checks at the call sites in env.cc?

@gireeshpunathil
Copy link
Member Author

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

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

@addaleax - it occurred to me now: the nullptr checks at the call sites in env.cc (the ones I removed) originally would mean that trace_controller can be null, while my changes (the ones I added) mandate that it cannot be null. Both cannot be true, so which is right? are there scenarios where the object is allowed to be null?

@gireeshpunathil
Copy link
Member Author

never mind, I don't see such as possibility in the code (trace_controller null being a valid use case). On the other hand, access from the base object method in the upstream code mandates a null check, so I guess we are good here.

@gireeshpunathil
Copy link
Member Author

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

@gireeshpunathil
src: nullcheck on trace controller
9605060 
Insert a NULLCHECK prior to return. Ideally we do this in the caller,
but the TraceController object is somewhat special as:
1. It is accessed by most threads
2. It's life cycle is managed by Agent::Agent
3. It's getter is invoked through Base Methods (upstream)

Refs: nodejs#25814
@gireeshpunathil
Copy link
Member Author

despite the full CI result showing all green, the Travis CI was showing error, and looks like there is no way to re-run it. So just squashed the commits and pushed that forced a build.

@addaleax
Copy link
Member

addaleax commented Feb 8, 2019

@gireeshpunathil I don’t know if it’s restricted to org admins – that would be kinda surprising – but generally you can re-run failing Travis builds. I’d ignore Travis failures if they aren’t related, though; sometimes it’s impossible to e.g. please the commit message linter.

Landed in c70e853 :)

@addaleax addaleax closed this Feb 8, 2019
pull bot pushed a commit to Rachelmorrell/node that referenced this pull request Feb 8, 2019
@gireeshpunathil @addaleax
src: nullcheck on trace controller
c70e853 
Insert a NULLCHECK prior to return. Ideally we do this in the caller,
but the TraceController object is somewhat special as:
1. It is accessed by most threads
2. It's life cycle is managed by Agent::Agent
3. It's getter is invoked through Base Methods (upstream)

Refs: nodejs#25814

PR-URL: nodejs#25943
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
@gireeshpunathil @addaleax
src: nullcheck on trace controller
e0af205 
Insert a NULLCHECK prior to return. Ideally we do this in the caller,
but the TraceController object is somewhat special as:
1. It is accessed by most threads
2. It's life cycle is managed by Agent::Agent
3. It's getter is invoked through Base Methods (upstream)

Refs: #25814

PR-URL: #25943
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shisama shisama shisama approved these changes

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gireeshpunathil @nodejs-github-bot @addaleax @jasnell @richardlau @shisama