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.

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

src: handle empty Maybe in uv binding initialize #25079

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@addaleax
Copy link
Member

addaleax commented Dec 17, 2018

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

Refs: #25061 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
src: handle empty Maybe in uv binding initialize
This can fail when terminating a Worker that loads
the `uv` binding at the same time.

Refs: #25061 (comment)

@addaleax addaleax requested a review from gireeshpunathil Dec 17, 2018

@danbev

danbev approved these changes Dec 17, 2018

@danbev

This comment has been minimized.

Copy link
Member

danbev commented Dec 17, 2018

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 19, 2018

Landed in 67f9582

@addaleax addaleax closed this Dec 19, 2018

@addaleax addaleax deleted the addaleax:uv-binding-map-set branch Dec 19, 2018

addaleax added a commit that referenced this pull request Dec 19, 2018

src: handle empty Maybe in uv binding initialize
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>
@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Dec 19, 2018

Does this mean we need to treat bindings loaded in workers with care for situations like this? (And this one fails more often because the error map creation is a lot of code when expanded?)

(Also I think we should be able to build that lazily...)
(EDIT: oh no, we also have the constants defined on the binding that are probably relied on by the users...)

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 19, 2018

@joyeecheung I think the reason for this is that Map operations call into builtin function, i.e. it is similar to calling JS.

I mean, I would personally prefer to always code more defensively, but I assume people have differing opinions on this…

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Dec 19, 2018

@addaleax I think it makes sense to be more defensive at least for the binding initializers - if I understand correctly, workers are special because now you have the ability to terminate a thread while it's bootstrapping (whereas previously things were more deterministic on the main thread?)

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 19, 2018

@joyeecheung Yes, exactly. I still plan to go through all of our source code and look for problematic cases… which is, unfortunately, a lot of code to go through 😄

MylesBorins added a commit that referenced this pull request Dec 25, 2018

src: handle empty Maybe in uv binding initialize
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>

@MylesBorins MylesBorins referenced this pull request Dec 25, 2018

Merged

v11.6.0 proposal #25175

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

src: handle empty Maybe in uv binding initialize
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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment