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

oom/core dump in repl with buffer property tab completion #3136

Closed
brycebaril opened this issue Sep 30, 2015 · 12 comments
Closed

oom/core dump in repl with buffer property tab completion #3136

brycebaril opened this issue Sep 30, 2015 · 12 comments
Labels
good first issue Issues that are suitable for first-time contributors. repl Issues and PRs related to the REPL subsystem.

Comments

@brycebaril
Copy link
Contributor

Noticed with a sufficiently large Buffer (or ArrayBuffer) tab completion in the repl will crash the process with out of memory + core dump

$ node
> var b = Buffer(1e8)
undefined
> b.l
<--- Last few GCs --->

   10335 ms: Mark-sweep 1397.5 (1445.0) -> 1397.4 (1445.0) MB, 264.7 / 0 ms [allocation failure] [GC in old space requested].
   10652 ms: Mark-sweep 1397.5 (1445.0) -> 1397.4 (1445.0) MB, 263.0 / 0 ms [allocation failure] [GC in old space requested].
   11009 ms: Mark-sweep 1397.5 (1445.0) -> 1397.4 (1445.0) MB, 264.6 / 0 ms [last resort gc].
   11330 ms: Mark-sweep 1397.4 (1445.0) -> 1397.4 (1445.0) MB, 321.3 / 0 ms [last resort gc].


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x3bd157b37339 <JS Object>
    2: ObjectGetOwnPropertyKeys(aka ObjectGetOwnPropertyKeys) [native v8natives.js:~688] [pc=0x1e2e0302e4b3] (this=0x3bd157b04131 <undefined>,J=0x5c7b14041d1 <an Uint8Array with map 0x20a90d01d2d9>,as=48)
    3: getOwnPropertyNames [native v8natives.js:752] [pc=0x1e2e0302e31b] (this=0x3bd157b36b61 <JS Function Object (SharedFunctionInfo 0x3bd157b36ad1)>,J=0x5c7b14041d1 <an Uint8Array with map 0...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory
Aborted (core dumped)
$ node -pe 'var b = Buffer(1e8); b.length'
100000000
$ node -v
v4.1.1
@trevnorris trevnorris added the repl Issues and PRs related to the REPL subsystem. label Sep 30, 2015
@trevnorris
Copy link
Contributor

Pretty sure the issue is from the following in lib/repl.js:

function filteredOwnPropertyNames(obj) {
  if (!obj) return [];
  return Object.getOwnPropertyNames(obj).filter(intFilter);
}

As can be demonstrated by the following:

$ node -e 'Object.getOwnPropertyNames(new Buffer(1e8))'

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory

@harry8525
Copy link

Could this be why there are ton of these issues reported on git hub with version 4.0+?

CALL_AND_RETRY_LAST Allocation failed - process out of memory

I'm getting random reports for some internal tools we use and haven't been able to pin point what causes it. It is scary if its related to inspecting properties that are buffers...especially using tools like gulp (where we are seeing all the issues) that have pass buffers around all the time...

@trevnorris
Copy link
Contributor

@harry8525 Since Buffers are now iterable, it would be possible to run through the list of all properties and remove any that are indices. Would probably crawl, but at least it wouldn't die.

@Fishrock123 Fishrock123 added the good first issue Issues that are suitable for first-time contributors. label Jan 7, 2016
@tjwebb
Copy link

tjwebb commented Feb 13, 2016

I just ran into this using winston + repl: trailsjs/trailpack-repl#20

Logging a small-ish object works fine. If I try to log a larger object (a couple dozen ES6 classes) it fails.

@JordanMajd
Copy link

I'm investigating the issue.

@JordanMajd
Copy link

Edit: Seems I was mistaken:

@trevnorris filteredOwnPropertyNames seems to be the issue. Digging in further.

Original:

@trevnorris Thanks for the suggestions, but I'm afraid they are dead ends.

I've commented out all invocations of filteredOwnPropertyNames to see if it is the cause. The issue still persists.

In addition, you recommended run through the list of all properties and remove any that are indices. It seems that filteredOwnPropertyNames invokes intFilter which should remove any indices.

Thanks for those suggestions, I'm still investigating the issue and open to any more advice.

@JordanMajd
Copy link

So the issue has to do with Object.getOwnPropertyNames in filteredOwnPropertyNames returns such a massive list the repl runs out of memory. getOwnPropertyNames returns an array of the objects enumerable and nonenumerable properties.

I've tried replacing this line with more memory efficient custom code, and succeeded with enumerable properties but I'm having trouble figuring out an alternative way to effectively get the objects nonenumarable properties.

Any advice is welcome.

@bnoordhuis
Copy link
Member

@JordanMajd There is currently no good way to do that. Presumably V8 would need to grow a C++ API that either lets you iterate over non-enumerable properties, or fails gracefully when creating the property name array fails (by creating an empty handle.) Node would then need to be updated to make use of this new API.

@JordanMajd
Copy link

Thanks @bnoordhuis,

Follow up question, does tab completion need to have nonenumerable properties? If it doesn't, I already have a working solution for only tab completing enumerable properties.

@bnoordhuis
Copy link
Member

I'd say so. For example, methods are normally non-enumerable; only considering enumerable properties would break completion for Object.getOwn<TAB>.

@JordanMajd
Copy link

Great point, I had forgotten that methods were non-enumerable.

If anyone has advice on where to go from here I am all ears. Otherwise it would seem this issue is not going to be resolved anytime in the near future.

@XadillaX
Copy link
Contributor

XadillaX commented Jun 20, 2017

How about to do Buffer with special treatment?

eg.

function filteredOwnPropertyNames(obj) {
  if (!obj) return [];
  if (obj instanceof Buffer && obj.length > 1000) {
    return Object.getOwnPropertyNames(new Buffer(1)).filter(intFilter);
  }
  return Object.getOwnPropertyNames(obj).filter(intFilter);
}

and then do process.emitWarning.

XadillaX added a commit to XadillaX/node that referenced this issue Jun 20, 2017
If the buffer or array is too large to completion, make a dummy smallest
substitute object for it and emit a warning.

Fixes: nodejs#3136
addaleax pushed a commit that referenced this issue Jul 11, 2017
If the buffer or array is too large to completion, make a dummy smallest
substitute object for it and emit a warning.

PR-URL: #13817
Fixes: #3136
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
addaleax pushed a commit that referenced this issue Jul 18, 2017
If the buffer or array is too large to completion, make a dummy smallest
substitute object for it and emit a warning.

PR-URL: #13817
Fixes: #3136
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
If the buffer or array is too large to completion, make a dummy smallest
substitute object for it and emit a warning.

PR-URL: #13817
Fixes: #3136
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants