Fix to #27211 - Refactor RealEnvStore methods to use libuv methods for env operations by devasci · Pull Request #27310 · nodejs/node · GitHub

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]

Thanks for working on this! I left some comments.

LGTM with a nit

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

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

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

@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 :)

@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:

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

Awesome, this looks perfect!

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

😳 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?

@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).

I think the bug may be in SafeGetenv() because that's not really handling the returned maybe correctly?

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

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

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.

This is going to be unblocked when #29508 lands.

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

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

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>

Landed in b354d12 🎉

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

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>

Landed in b354d12 🎉

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

Thank you so much 😁 👍🏻

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>

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

devasci mentioned this pull request Apr 19, 2019

Refactor RealEnvStore methods to use uv_os_getenv/setenv/unsetenv #27211

Closed

devsnek approved these changes Apr 19, 2019

View reviewed changes

devsnek reviewed Apr 19, 2019

View reviewed changes

Comment thread src/node_env_var.cc Outdated

joyeecheung reviewed Apr 19, 2019

View reviewed changes

Comment thread src/node_env_var.cc Outdated

Comment thread src/node_env_var.cc Outdated

Comment thread src/node_env_var.cc Outdated

Comment thread src/node_env_var.cc Outdated

joyeecheung reviewed Apr 19, 2019

View reviewed changes

Comment thread src/node_env_var.cc Outdated

devasci force-pushed the d3v-dev branch from 7d1720d to 1936655 Compare April 19, 2019 17:30

joyeecheung approved these changes Apr 19, 2019

View reviewed changes

Comment thread src/node_env_var.cc Outdated

addaleax reviewed Apr 19, 2019

View reviewed changes

Comment thread src/node_env_var.cc Outdated

Comment thread src/node_env_var.cc Outdated

Comment thread src/node_env_var.cc Outdated

Comment thread src/node_env_var.cc Outdated

Comment thread src/node_env_var.cc Outdated

Comment thread src/node_env_var.cc Outdated

devasci force-pushed the d3v-dev branch from 4bc0fb4 to d49f5af Compare April 20, 2019 18:24

addaleax approved these changes Apr 20, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 20, 2019

joyeecheung reviewed Apr 21, 2019

View reviewed changes

Comment thread src/node_env_var.cc Outdated

Comment thread src/node_env_var.cc Outdated

BridgeAR removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 21, 2019

joyeecheung reviewed Apr 23, 2019

View reviewed changes

Comment thread src/node_env_var.cc Outdated

addaleax added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Sep 12, 2019

addaleax closed this Sep 12, 2019

BridgeAR mentioned this pull request Sep 24, 2019

v12.11.0 proposal #29695

Merged

cjihrig mentioned this pull request Dec 27, 2019

Assertion failed: len < MAX_ENV_VAR_LENGTH, file c:\ws\deps\uv\src\win\util.c, line 1476 libuv/libuv#2587

Closed

bl-ue mentioned this pull request Apr 18, 2020

src: fix empty-named env var assertion failure #32921

Closed

3 tasks

Fix to #27211 - Refactor RealEnvStore methods to use libuv methods for env operations by devasci · Pull Request #27310 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

Fix to #27211 - Refactor RealEnvStore methods to use libuv methods for env operations#27310devasci wants to merge 10 commits intonodejs:masternodejs/node:masterfrom devasci:d3v-devdevasci/node:d3v-devCopy head branch name to clipboard

Conversation

devasci commented Apr 19, 2019

Checklist

Uh oh!

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devasci commented Apr 19, 2019

Uh oh!

devasci commented Apr 19, 2019

Uh oh!

addaleax commented Apr 19, 2019

Uh oh!

devasci commented Apr 20, 2019

Uh oh!

devasci commented Apr 20, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 20, 2019

Uh oh!

devasci commented Apr 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Apr 21, 2019

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 23, 2019

Uh oh!

nodejs-github-bot commented Apr 23, 2019

Uh oh!

Uh oh!

addaleax commented Aug 20, 2019

Uh oh!

addaleax commented Sep 9, 2019

Uh oh!

nodejs-github-bot commented Sep 12, 2019

Uh oh!

nodejs-github-bot commented Sep 12, 2019

Uh oh!

addaleax commented Sep 12, 2019

Uh oh!

devasci commented Oct 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Fix to #27211 - Refactor RealEnvStore methods to use libuv methods for env operations#27310
devasci wants to merge 10 commits into
nodejs:masterfrom
devasci:d3v-dev

devasci commented Apr 20, 2019 •
edited
Loading