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

[backport v10.x] src: handle empty Maybe in uv binding initialize #28832

Closed
wants to merge 1 commit into from
Closed

[backport v10.x] src: handle empty Maybe in uv binding initialize #28832

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 24, 2019

This can fail when terminating a Worker that loads
the uv binding at the same time.

Refs: #25061 (comment)
Fixes: #25134
PR-URL: #25079
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Daniel Bevenius daniel.bevenius@gmail.com
Reviewed-By: Gireesh Punathil gpunathi@in.ibm.com
Reviewed-By: Colin Ihrig cjihrig@gmail.com

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. libuv Issues and PRs related to the libuv dependency or the uv binding. v10.x labels Jul 24, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott Trott closed this Jul 24, 2019
addaleax
addaleax reviewed Jul 24, 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.

I think this would make it work

src/uv.cc Outdated Show resolved Hide resolved
@Trott Trott reopened this Jul 26, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@addaleax @Trott
src: handle empty Maybe in uv binding initialize
ca4a22e 
This can fail when terminating a Worker that loads
the `uv` binding at the same time.

Refs: nodejs#25061 (comment)
Fixes: nodejs#25134
PR-URL: nodejs#25079
Backport-PR-URL: nodejs#28832
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 26, 2019 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/24715/ ✔️

This was referenced Jul 26, 2019
Closed
Closed
BethGriggs pushed a commit that referenced this pull request Jul 30, 2019
@addaleax @BethGriggs
src: handle empty Maybe in uv binding initialize
0339fba 
This can fail when terminating a Worker that loads
the `uv` binding at the same time.

Refs: #25061 (comment)
Fixes: #25134
PR-URL: #25079
Backport-PR-URL: #28832
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@BethGriggs
Copy link
Member

Landed on v10.x-staging

@BethGriggs BethGriggs closed this Jul 30, 2019
@Trott Trott deleted the backport-25079 branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Trott @nodejs-github-bot @BethGriggs @addaleax