src: use std::optional for Worker thread id #41453

addaleax · 2022-01-09T18:55:39Z

addaleax · 2022-01-09T18:56:19Z

src/node_worker.h


  std::unique_ptr<InspectorParentHandle> inspector_parent_handle_;

  // This mutex protects access to all variables listed below it.
  mutable Mutex mutex_;

-  bool thread_joined_ = true;


The comment here implied that this was protected by the mutex, but a) that didn’t reflect the actual usage of this field and b) it’s only ever accessed from the parent thread anyway, so no locking is necessary here

mhdawson

LGTM

nodejs-github-bot · 2022-01-13T14:24:51Z

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

nodejs-github-bot · 2022-01-13T20:09:15Z

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

tniessen

Still LGTM

nodejs-github-bot · 2022-01-14T06:39:23Z

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

nodejs-github-bot · 2022-01-14T10:22:46Z

Commit Queue failed

- Loading data for nodejs/node/pull/41453
✔  Done loading data for nodejs/node/pull/41453
----------------------------------- PR info ------------------------------------
Title      src: use std::optional for Worker thread id (#41453)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     addaleax:41421-response -> nodejs:master
Labels     c++, author ready, worker
Commits    2
 - src: use std::optional for Worker thread id
 - fixup! src: use std::optional for Worker thread id
Committers 2
 - Anna Henningsen 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/41453
Refs: https://github.com/nodejs/node/pull/41421
Reviewed-By: Tobias Nießen 
Reviewed-By: Rich Trott 
Reviewed-By: Colin Ihrig 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41453
Refs: https://github.com/nodejs/node/pull/41421
Reviewed-By: Tobias Nießen 
Reviewed-By: Rich Trott 
Reviewed-By: Colin Ihrig 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 09 Jan 2022 18:55:39 GMT
   ✔  Approvals: 4
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/41453#pullrequestreview-852398248
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/41453#pullrequestreview-847354707
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/41453#pullrequestreview-848035956
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/41453#pullrequestreview-848089907
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2022-01-14T06:39:23Z: https://ci.nodejs.org/job/node-test-pull-request/41866/
- Querying data for job/node-test-pull-request/41866/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/1697209985

nodejs-github-bot · 2022-01-14T14:47:56Z

Commit Queue failed

- Loading data for nodejs/node/pull/41453
✔  Done loading data for nodejs/node/pull/41453
----------------------------------- PR info ------------------------------------
Title      src: use std::optional for Worker thread id (#41453)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     addaleax:41421-response -> nodejs:master
Labels     c++, author ready, worker, dont-land-on-v12.x, dont-land-on-v14.x, dont-land-on-v16.x
Commits    2
 - src: use std::optional for Worker thread id
 - fixup! src: use std::optional for Worker thread id
Committers 2
 - Anna Henningsen 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/41453
Refs: https://github.com/nodejs/node/pull/41421
Reviewed-By: Tobias Nießen 
Reviewed-By: Rich Trott 
Reviewed-By: Colin Ihrig 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41453
Refs: https://github.com/nodejs/node/pull/41421
Reviewed-By: Tobias Nießen 
Reviewed-By: Rich Trott 
Reviewed-By: Colin Ihrig 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 09 Jan 2022 18:55:39 GMT
   ✔  Approvals: 4
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/41453#pullrequestreview-852398248
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/41453#pullrequestreview-847354707
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/41453#pullrequestreview-848035956
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/41453#pullrequestreview-848089907
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2022-01-14T10:22:46Z: https://ci.nodejs.org/job/node-test-pull-request/41866/
- Querying data for job/node-test-pull-request/41866/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/1698193651

RaisinTen

🚀

aduh95 · 2022-01-14T16:56:27Z

Landed in c22177f

Refs: #41421 PR-URL: #41453 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Darshan Sen <[email protected]>

Refs: nodejs#41421 PR-URL: nodejs#41453 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Darshan Sen <[email protected]>

src: use std::optional for Worker thread id

f177794

Refs: nodejs#41421

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Jan 9, 2022

addaleax commented Jan 9, 2022

View reviewed changes

addaleax added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Jan 9, 2022

addaleax mentioned this pull request Jan 9, 2022

worker: add missing initialization #41421

Closed

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 9, 2022

tniessen approved these changes Jan 9, 2022

View reviewed changes

Trott approved these changes Jan 10, 2022

View reviewed changes

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2022

cjihrig approved these changes Jan 10, 2022

View reviewed changes

mhdawson approved these changes Jan 10, 2022

View reviewed changes

tniessen added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 13, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2022

fixup! src: use std::optional for Worker thread id

8eb8c70

github-actions bot mentioned this pull request Jan 14, 2022

CI Reliability 2022-01-14 nodejs/reliability#173

Open

22 tasks

tniessen approved these changes Jan 14, 2022

View reviewed changes

addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 14, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 14, 2022

richardlau added dont-land-on-v12.x labels Jan 14, 2022

richardlau added dont-land-on-v16.x commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 14, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 14, 2022

RaisinTen approved these changes Jan 14, 2022

View reviewed changes

aduh95 merged commit c22177f into nodejs:master Jan 14, 2022

github-actions bot mentioned this pull request Jan 15, 2022

CI Reliability 2022-01-15 nodejs/reliability#174

Open

26 tasks

targos mentioned this pull request Jan 16, 2022

v17.4.0 release proposal #41557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: use std::optional for Worker thread id #41453

src: use std::optional for Worker thread id #41453

addaleax commented Jan 9, 2022

addaleax Jan 9, 2022

mhdawson left a comment

nodejs-github-bot commented Jan 13, 2022

nodejs-github-bot commented Jan 13, 2022

tniessen left a comment

nodejs-github-bot commented Jan 14, 2022

nodejs-github-bot commented Jan 14, 2022

nodejs-github-bot commented Jan 14, 2022

RaisinTen left a comment

aduh95 commented Jan 14, 2022

src: use std::optional for Worker thread id #41453

src: use std::optional for Worker thread id #41453

Conversation

addaleax commented Jan 9, 2022

addaleax Jan 9, 2022

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 13, 2022

nodejs-github-bot commented Jan 13, 2022

tniessen left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 14, 2022

nodejs-github-bot commented Jan 14, 2022

nodejs-github-bot commented Jan 14, 2022

RaisinTen left a comment

Choose a reason for hiding this comment

aduh95 commented Jan 14, 2022