GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: minor worker code cleanups by addaleax · Pull Request #32562 · nodejs/node

Conversation

addaleax commented Mar 30, 2020

Copy link

Copy Markdown

Member

src: clean up worker thread creation code

Instead of setting and then in the case of error un-setting properties,
only set them when no error occurs.

Refs: #32344

src: remove loop_init_failed_ from Worker class

There’s no reason for this to not be stored alongside the loop
data structure itself.

Checklist

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction

addaleax added 2 commits

March 30, 2020 10:36


          src: clean up worker thread creation code

09d3469

Instead of setting and then in the case of error un-setting properties,
only set them when no error occurs.

Refs: nodejs#32344


          src: remove loop_init_failed_ from Worker class

09003e3

There’s no reason for this to not be stored alongside the loop
data structure itself.

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

worker Issues and PRs related to the worker_threads module and Worker API. labels

Mar 30, 2020

nodejs-github-bot commented Mar 30, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

HarshithaKP reviewed

Mar 30, 2020

View reviewed changes

Comment thread

src/node_worker.cc

		w->loop_init_failed_ = true;
		w->thread_joined_ = true;
		w->stopped_ = true;
		w->env()->remove_sub_worker_context(w);

HarshithaKP Mar 30, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why these assignments are removed ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax Mar 30, 2020

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@HarshithaKP Because they either had no effect or were just un-setting variables that were set above.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

himself65 and HarshithaKP reacted with thumbs up emoji

All reactions

👍 2 reactions

cjihrig approved these changes

Mar 30, 2020

View reviewed changes

lundibundi approved these changes

Mar 30, 2020

View reviewed changes

HarshithaKP commented Mar 30, 2020

Copy link

Copy Markdown

Member

LGTM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Mar 30, 2020

nodejs-github-bot commented Mar 30, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell approved these changes

Mar 30, 2020

View reviewed changes

nodejs-github-bot commented Apr 2, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax commented Apr 2, 2020

Copy link

Copy Markdown

Member Author

Landed in 0c70e8c...f558a76

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax closed this

Apr 2, 2020

addaleax added a commit that referenced this pull request

Apr 2, 2020


          src: clean up worker thread creation code

be87651

Instead of setting and then in the case of error un-setting properties,
only set them when no error occurs.

Refs: #32344

PR-URL: #32562
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

addaleax added a commit that referenced this pull request

Apr 2, 2020


          src: remove loop_init_failed_ from Worker class

f558a76

There’s no reason for this to not be stored alongside the loop
data structure itself.

PR-URL: #32562
Refs: #32344
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

addaleax deleted the worker-cleanup branch

April 2, 2020 16:14

BethGriggs pushed a commit that referenced this pull request

Apr 7, 2020


          src: clean up worker thread creation code

cf73be1

Instead of setting and then in the case of error un-setting properties,
only set them when no error occurs.

Refs: #32344

PR-URL: #32562
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

BethGriggs pushed a commit that referenced this pull request

Apr 7, 2020


          src: remove loop_init_failed_ from Worker class

387c63a

There’s no reason for this to not be stored alongside the loop
data structure itself.

PR-URL: #32562
Refs: #32344
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

targos pushed a commit that referenced this pull request

Apr 12, 2020


          src: clean up worker thread creation code

0faaa7c

Instead of setting and then in the case of error un-setting properties,
only set them when no error occurs.

Refs: #32344

PR-URL: #32562
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

targos pushed a commit that referenced this pull request

Apr 12, 2020


          src: remove loop_init_failed_ from Worker class

4df3ac2

There’s no reason for this to not be stored alongside the loop
data structure itself.

PR-URL: #32562
Refs: #32344
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

targos mentioned this pull request

Apr 13, 2020

v13.13.0 release proposal #32813

Merged

targos added backport-blocked-v12.x and removed author ready

PRs with CI started, the required approvals, and no outstanding review comments.

backport-blocked-v12.x labels

Apr 22, 2020

targos pushed a commit to targos/node that referenced this pull request

Apr 25, 2020


          src: clean up worker thread creation code

fa41f32

Instead of setting and then in the case of error un-setting properties,
only set them when no error occurs.

Refs: nodejs#32344

PR-URL: nodejs#32562
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

targos pushed a commit to targos/node that referenced this pull request

Apr 25, 2020


          src: remove loop_init_failed_ from Worker class

2847ab2

There’s no reason for this to not be stored alongside the loop
data structure itself.

PR-URL: nodejs#32562
Refs: nodejs#32344
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

targos pushed a commit that referenced this pull request

Apr 28, 2020


          src: clean up worker thread creation code

9f6ed72

Instead of setting and then in the case of error un-setting properties,
only set them when no error occurs.

Refs: #32344

PR-URL: #32562
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

targos pushed a commit that referenced this pull request

Apr 28, 2020


          src: remove loop_init_failed_ from Worker class

464ff85

There’s no reason for this to not be stored alongside the loop
data structure itself.

PR-URL: #32562
Refs: #32344
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>

targos mentioned this pull request

May 2, 2020

v12.17.0 release proposal #33197

Merged

Reviewers

HarshithaKP

HarshithaKP left review comments

jasnell

jasnell approved these changes

cjihrig

cjihrig approved these changes

+1 more reviewer

lundibundi

lundibundi approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++

Issues and PRs that require attention from people who are familiar with C++.

worker

Issues and PRs related to the worker_threads module and Worker API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.