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.

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

Fix to #27211 - Refactor RealEnvStore methods to use libuv methods for env operations #27310

Closed
wants to merge 10 commits into from

Conversation

devasci
Copy link
Contributor

@devasci devasci commented Apr 19, 2019

Refactored RealEnvStore::Get, Set, Query and Delete methods to use uv_os_getenv/setenv/unsetenv methods to avoid conditional OS switches.

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 19, 2019
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I left some comments.

src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

src/node_env_var.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
@devasci
Copy link
Contributor Author

devasci commented Apr 19, 2019

Thank you @addaleax and @joyeecheung for the review comments. i'll continue to work on these review and will push changes soon.

@devasci
Copy link
Contributor Author

devasci commented Apr 19, 2019

@addaleax could you elaborate a little more about the comment you and Joyee Cheung are discussing about changing return type to MaybeLocal?

@addaleax
Copy link
Member

@devasci I don’t think it’s strictly blocking, because nothing crashes and it’s unlikely that we actually run into errors.

In this PR, you’re adding new error conditions depending on the return code we get from libuv. There are multiple ways we could deal with that, e.g. returning undefined from the getter (which this PR currently ends up doing, but in a somewhat unusual way), returning an empty string from the getter (which would be String::Empty() rather than Local<String>()), or throwing an exception.

When we throw an exception from a C++ function that is not directly exposed to JS, like RealEnvStore::Get(), the usual way to do that is to make the function return a MaybeLocal<> instead of a Local<>, and in the case of an error, throw an exception and return an empty handle (i.e. MaybeLocal<String>()) . Then, in the calling function (here that’s EnvGetter()), we check whether the return value is an empty MaybeLocal<>(), and if it is, we return to JS as quickly as possible to make the exception visible.

The typical way to throw an exception for a libuv error is to use env->ThrowUVException(libuv_error_ode, "function_name");, e.g. in this case env->ThrowUVException(ret, "getenv");. env is the current Environment* instance; you can get it through env = Environment::GetCurrent(isolate); in this case (or update the KVStore functions to take an Environment* instead of an Isolate* pointer in order to have the Environment* instance available – that might make sense anyway).

@joyeecheung If I have forgotten something, feel free to add that :)

@devasci
Copy link
Contributor Author

devasci commented Apr 20, 2019

@addaleax thank you for writing very clearly, that helped me a lot to understand node code better.

I've been working on the review comments you provided, most of them are fixed now. But i'm stuck with some error popping up while running make lint and make test.

I tried to see possible causes of the error but i'm unable to fix that. Please help me out here.

I get the below error, please assist me what could have been wrong:
Screenshot 2019-04-20 at 19 54 53
Screenshot 2019-04-20 at 19 55 16

@devasci
Copy link
Contributor Author

devasci commented Apr 20, 2019

got the issue and fixed it, tests are passing now.

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.

Awesome, this looks perfect!

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 20, 2019
@nodejs-github-bot
Copy link
Collaborator

@devasci
Copy link
Contributor Author

devasci commented Apr 20, 2019

😳 all tests are failing, looks like out/doc/applinks.json Error is back.
Need some assistance to understand the error and fix it.

If we do not throw UVException from RealEnv::Get method tests are passing. Should we dont throw any exception from RealEnv::Get method?

@joyeecheung
Copy link
Member

@devasci The error is not related to doc tools, it indicates that the binary crashes on start up (which means there's a bug in the Get imeplementation - process.env getter is used a lot during bootstrap).

Copy link
Member

@joyeecheung joyeecheung 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 the bug may be in SafeGetenv() because that's not really handling the returned maybe correctly?

src/node_env_var.cc Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 21, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

src/node_env_var.cc Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

I’ve rebased this and kicked off a new CI. That should only fail on Windows and work once we have libuv/libuv#2419 in Node.js.

@addaleax
Copy link
Member

addaleax commented Sep 9, 2019

This is going to be unblocked when #29508 lands.

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Sep 12, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Sep 12, 2019
Modified RealEnvStore::Get, Set, Query and Delete methods
to use libuv methods environment variables operations instead
of using os specific logic and switches.

Fixes: #27211
Refs: http://docs.libuv.org/en/v1.x/misc.html

PR-URL: #27310
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

Landed in b354d12 🎉

@devasci Sorry it took so long, but congratulations to your first contribution!

@addaleax addaleax closed this Sep 12, 2019
targos pushed a commit that referenced this pull request Sep 20, 2019
Modified RealEnvStore::Get, Set, Query and Delete methods
to use libuv methods environment variables operations instead
of using os specific logic and switches.

Fixes: #27211
Refs: http://docs.libuv.org/en/v1.x/misc.html

PR-URL: #27310
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
Modified RealEnvStore::Get, Set, Query and Delete methods
to use libuv methods environment variables operations instead
of using os specific logic and switches.

Fixes: #27211
Refs: http://docs.libuv.org/en/v1.x/misc.html

PR-URL: #27310
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@devasci
Copy link
Contributor Author

devasci commented Oct 3, 2019

Landed in b354d12 🎉

@devasci Sorry it took so long, but congratulations to your first contribution!

Thank you so much 😁 👍🏻

addaleax pushed a commit that referenced this pull request Apr 27, 2020
Setting an environment variable with an empty name on Windows resulted
in an assertion failure, because it was checked for an '=' sign at the
beginning without verifying the length was greater than 0.

Fixes: #32920
Refs: #27310

PR-URL: #32921
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
Setting an environment variable with an empty name on Windows resulted
in an assertion failure, because it was checked for an '=' sign at the
beginning without verifying the length was greater than 0.

Fixes: #32920
Refs: #27310

PR-URL: #32921
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
Setting an environment variable with an empty name on Windows resulted
in an assertion failure, because it was checked for an '=' sign at the
beginning without verifying the length was greater than 0.

Fixes: #32920
Refs: #27310

PR-URL: #32921
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
Setting an environment variable with an empty name on Windows resulted
in an assertion failure, because it was checked for an '=' sign at the
beginning without verifying the length was greater than 0.

Fixes: #32920
Refs: #27310

PR-URL: #32921
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
Setting an environment variable with an empty name on Windows resulted
in an assertion failure, because it was checked for an '=' sign at the
beginning without verifying the length was greater than 0.

Fixes: #32920
Refs: #27310

PR-URL: #32921
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
Setting an environment variable with an empty name on Windows resulted
in an assertion failure, because it was checked for an '=' sign at the
beginning without verifying the length was greater than 0.

Fixes: #32920
Refs: #27310

PR-URL: #32921
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2020
Setting an environment variable with an empty name on Windows resulted
in an assertion failure, because it was checked for an '=' sign at the
beginning without verifying the length was greater than 0.

Fixes: #32920
Refs: #27310

PR-URL: #32921
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
targos pushed a commit that referenced this pull request May 13, 2020
Setting an environment variable with an empty name on Windows resulted
in an assertion failure, because it was checked for an '=' sign at the
beginning without verifying the length was greater than 0.

Fixes: #32920
Refs: #27310

PR-URL: #32921
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants