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

Is this posible/reachabe code? #2600

Closed
juanarbol opened this issue Apr 12, 2020 · 7 comments
Closed

Is this posible/reachabe code? #2600

juanarbol opened this issue Apr 12, 2020 · 7 comments

Comments

@juanarbol
Copy link
Member

juanarbol commented Apr 12, 2020

  • Node.js Version: current
  • OS: any
  • Scope (install, code, runtime, meta, other?):
  • Module (and version) (if relevant): current
// src/node_os.cc:133
static void GetFreeMemory(const FunctionCallbackInfo<Value>& args) {
  double amount = uv_get_free_memory();
  if (amount < 0) // This
    return;
  args.GetReturnValue().Set(amount);
}

Is it possible to have amount < 0 ? Based on libuv codebase (at least on LINUX), the free mem (also total mem) is taken from /proc/meminfo, based on that, is it possible to have a negative amount?

@gireeshpunathil
Copy link
Member

@juanarbol
Copy link
Member Author

Yeap, I think it is a possible "non-positive amount", in that case, don't you think that we should provide a better error handling on the implementation? I mean, at least use CHECK_GT or something like that...

I don't know, this is just a proposal because I don't know why we are dealing with invalid amount in this way.

@gireeshpunathil
Copy link
Member

don't you think that we should provide a better error handling on the implementation?

@juanarbol - makes sense to me! but throw an exception that carries the original libuv error message, with a doc update, instead of CHECK_GT?

@juanarbol
Copy link
Member Author

Yeap, it is a better approach, I think that:

if (amount < 0) {
  return uv_err_name(amount);
}

But amm... the free memory method is void so that won't work, and I don't know how to throw an exception with a given FunctionCallbackInfo

@gireeshpunathil
Copy link
Member

here is a recent PR which did a similar thing: you need to:

  • get the error message from libuv,
  • compose a full exception message with that,
  • define a new error type if need be, in src/node_errors.h
  • and throw

https://github.com/nodejs/node/pull/32344/files#diff-853c968290ed44d71c73689242b1fab4R648-R661

/cc @addaleax for a better suggestion!

@bnoordhuis
Copy link
Member

Is it possible to have amount < 0?

No, because the return type of uv_get_free_memory() is uint64_t, i.e, an unsigned value. Those are always >= 0.

juanarbol added a commit to juanarbol/node that referenced this issue Apr 13, 2020
Based on nodejs/help#2600 (comment)
the condition (amount < 0) won't be possible.
@juanarbol
Copy link
Member Author

Thanks for the support! I'm closing this issue now.

juanarbol added a commit to nodejs/node that referenced this issue Apr 19, 2020
Based on nodejs/help#2600 (comment)
the condition (amount < 0) won't be possible.

PR-URL: #32818
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
BethGriggs pushed a commit to nodejs/node that referenced this issue Apr 27, 2020
Based on nodejs/help#2600 (comment)
the condition (amount < 0) won't be possible.

PR-URL: #32818
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
BridgeAR pushed a commit to nodejs/node that referenced this issue Apr 28, 2020
Based on nodejs/help#2600 (comment)
the condition (amount < 0) won't be possible.

PR-URL: #32818
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
targos pushed a commit to nodejs/node that referenced this issue Apr 30, 2020
Based on nodejs/help#2600 (comment)
the condition (amount < 0) won't be possible.

PR-URL: #32818
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants