YuriGor / deepdash

eachDeep, filterDeep, findDeep, someDeep, omitDeep, pickDeep, keysDeep etc.. Tree traversal library written in Underscore/Lodash fashion
https://deepdash.io/
MIT License
274 stars 12 forks source link

Iterating over inherited props - unexpected breaking change in 5.2 #65

Closed TeleMediaCC closed 4 years ago

TeleMediaCC commented 4 years ago

<--- Last few GCs --->

[10820:00000178C3D42B80] 15484 ms: Mark-sweep 1959.0 (2095.8) -> 1959.0 (2095.8) MB, 470.1 / 0.0 ms (average mu = 0.076, current mu = 0.000) allocation failure scavenge might not succeed [10820:00000178C3D42B80] 15786 ms: Mark-sweep 1959.0 (2095.8) -> 1959.0 (2095.8) MB, 302.8 / 0.0 ms (average mu = 0.044, current mu = 0.000) allocation failure scavenge might not succeed

<--- JS stacktrace --->

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

Security context: 0x00789f1408d1 0: builtin exit frame: concat(this=0x036ca3d71271 <JSArray[6552]>,0x019ed21f20a9 <JSArray[1]>,0x019ed21fcb59 <JSArray[0]>,0x036ca3d71271 <JSArray[6552]>)

1: getOwnChildren(aka getOwnChildren) [000002FB79D26571] [P:\DEV\CC\CC\node_modules\deepdash\private\getIterate.js:~211] [pc=0000019BD302EF67](this=0x01909d2804b1 <undefined>,0x02fb79d0bd51 <Object map = 000002306D5E2C19>,0x007db7dff45...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

YuriGor commented 4 years ago

Hi, could you please provide some details how to reproduce this?

YuriGor commented 4 years ago

Also make sure your data have no circular references or activate checkCircular option.

TeleMediaCC commented 4 years ago

Can't create reproduce link. Its part of a big nuxt.js projekt. I upgraded 5.1.2 to 5.2.0 and after start immediately crashed the app. When i downgrade to 5.1.2, everything works.

YuriGor commented 4 years ago
  1. Try to set checkCircular option: It probably started visiting some paths, ignored previously, and stuck in some circular ref.
  2. Share your usage examples: which deepdash methods, what options, amount, and structure of data?
YuriGor commented 4 years ago

Let me know if any news please, closing for now because of no way to reproduce.

Ilshidur commented 4 years ago

I had this issue while doing a recursive lookup in a Mongoose document. I had to prevent this lib to use the internal fields like so :

const deepdash = require('deepdash/standalone');

const myMongooseObject = Model.find(...);

// `myMongooseObject.randomArray` is a Mongoose sub-schema.

deepdash.eachDeep(myMongooseObject.randomArray, ...); // Crashes.
deepdash.eachDeep(myMongooseObject.toJSON().randomArray, ...); // No crash.

I hope it can help whoever steps on this issue.

YuriGor commented 4 years ago

Are you able to check if this works?

deepdash.eachDeep(myMongooseObject.randomArray, ()=>{}, { checkCircular: true });
Ilshidur commented 4 years ago

Oh, my apologies, I did not provide the options in my last message.

{
  leavesOnly: false,
  checkCircular: true,
  pathFormat: 'string',
  children: ['main', 'sub.main'],
}

It works with these options.

YuriGor commented 4 years ago

Hi, thanks, but could you please check it works with these options but without converting mongo doc to json?

Ilshidur commented 4 years ago

Hi. I was able to reproduce the issue. However, I don't know where this could be from, neither whether this is an issue on my end or not.


Here's the most minimalist way I found to reproduce this behavior :

const { eachDeep } = require('deepdash/standalone');
const mongoose = require('mongoose');

const RandomSchema = mongoose.Schema({ items: [mongoose.Schema()] });
const RandomModel = mongoose.model('random', RandomSchema);

function demo() {
  const myDocument = new RandomModel({
    items: [{}],
  });

  eachDeep(myDocument.items);
}

demo();

Here's my console afterwards :

<--- Last few GCs --->
e [15516:0x45eb700]     6027 ms: Mark-sweep 2038.1 (2070.2) -> 2037.9 (2074.7) MB, 68.1 / 0.0 ms  (+ 257.0 ms in 62 steps since start of marking, biggest step 13.6 ms, walltime since start of marking 332 ms) (average mu = 0.103, current mu = 0.021) finalize[15516:0x45eb700]     6416 ms: Mark-sweep 2042.7 (2074.7) -> 2042.5 (2078.2) MB, 78.0 / 0.0 ms  (+ 300.9 ms in 58 steps since start of marking, biggest step 14.5 ms, walltime since start of marking 388 ms) (average mu = 0.064, current mu = 0.026) finalize

<--- JS stacktrace --->

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

    0: ExitFrame [pc: 0x1a5c1a2]
Security context: 0x2a489939a299 <JSObject>
    1: push [0x2a489938bf11](this=0x2fc204050611 <JSArray[59]>,0x09ae6197e8f9 <Object map = 0x2971eab8cde9>)
    2: getOwnChildren(aka getOwnChildren) [0x2da1d8ea3289] [/home/ncoutin/Documents/projects/roma-carts/node_modules/deepdash/private/getIterate.js:~211] [pc=0x7720ae9625d](this=0x058209e804d1 <undefined>,0x09ae6197c331 <Object map = 0x2971eab8d069>,0x0fa6...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0x97a1c0 node::Abort() [node]
 2: 0x97b189 node::OnFatalError(char const*, char const*) [node]
 3: 0xaf2d0e v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]
 4: 0xaf3089 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [node]
 5: 0xefe235  [node]
 6: 0xf08b9b v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [node]
 7: 0xf098b7 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 8: 0xf0af9f v8::internal::Heap::HandleGCRequest() [node]
 9: 0xea5890 v8::internal::StackGuard::HandleInterrupts() [node]
10: 0x11a90b5 v8::internal::Runtime_StackGuard(int, unsigned long*, v8::internal::Isolate*) [node]
11: 0x1a5c1a2  [node]
[1]    15516 abort (core dumped)  node index.js

Anyway, thank you very much for this lib. I use it at work and it really saved some of my time.

YuriGor commented 4 years ago

Thank you for this demo, there are two nuances:

  1. circular references
  2. HUGE amount of inherited properties provided by mongoose

So yes, it's a good idea to convert document to plain object before going deep.

Here is an illustrated version of your code:

const { eachDeep } = require('deepdash/standalone')
const mongoose = require('mongoose')

const RandomSchema = mongoose.Schema({ items: [mongoose.Schema()] })
const RandomModel = mongoose.model('random', RandomSchema)

function demo () {
  const myDocument = new RandomModel({
    items: [{}]
  })

  const has = Object.prototype.hasOwnProperty
  eachDeep(myDocument.items, (v, k, p, x) => {
    console.log(`${x.depth}: ${x.path}`)
    if (x.isCircular) {
      console.log(` - ${k} is a circular ref`)
    }
    if (!has.call(p, k)) {
      console.log(` - ${k} is inherited property`)
      return false // skip inherited props otherwize the whole universe will fall on you
    }
  }, { checkCircular: true })
}

demo()

[see output]

So as you see even with inherited props skipped it's still pretty big.

Since v5.2 for performance reasons I switched to for .. in way of iteration over objects instead of Lodash forOwn and decided to not check internally if property is own. So it was my mistake to bump only minor version, not major, because it's undocumented but breaking change.

I think I will add an option to iterate over own properties only, true by default, so it will get previous expected behavior back.

Thank you guys @TeleMediaCC @Ilshidur for your patience and help!

TeleMediaCC commented 4 years ago

I think the problem when the object contains prop name: $type

TeleMediaCC commented 4 years ago

I think the problem on the Mongoose.Types values. I have the same problem with mapValuesDeep.

YuriGor commented 4 years ago

In v5.3.0 new option widely added ownPropertiesOnly and it's true by default, so now all the methods should behave as they used to before v5.2.0 if ownPropertiesOnly is false - then deepdash will also go over prototype props. No time for docs :cry: