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

Memory Leak? native Promise and complex arrow functions with Generators #4210

Closed
LightSpeedC opened this issue Dec 9, 2015 · 5 comments
Closed
Labels
question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency.

Comments

@LightSpeedC
Copy link

Memory Leak? native Promise and complex arrow functions with Generators.

node version: v0.12.9, v4.2.3, v5.1.1, v5.2.0 (x86, x64)
I tried Windows version. OS: Windows 10 (64bit), Windows 8.1 (64bit).

following source causes Error.

using native Promise cause Error.
but, other Promise implementation such as npm "bluebird" or npm "promise-light", looks good.
or arrow function with block statement looks good.

// Memory Leak? native Promise and complex arrow functions with Generators
// node version: v0.12.9, v4.2.3, v5.1.1, v5.2.0 (x86, x64)
// I tried Windows version. OS: Windows 10 (64bit), Windows 8.1 (64bit).

'use strict';

//var Promise = require('promise-light'); // better Promise implementation
//var Promise = require('bluebird');      //  best  Promise implementation

var test = val => Promise.resolve(val);
var wait = sec => new Promise((res, rej) => setTimeout(res, sec * 1000));
var fork = gen => new Promise((res, rej, cb) => (
    cb = (err, val) => (
        (val = err ? gen.throw(err) : gen.next(val)),
        (val.done ? res(val.value) : val.value.then(val => cb(null, val), cb))
    ), cb()
));
var forkSolved = gen => new Promise((res, rej) => {
    var cb = (err, val) => {
        try { val = err ? gen.throw(err) : gen.next(val); } catch (e) { rej(e); }
        val.done ? res(val.value) : val.value.then(val => cb(null, val), cb);
    }; cb();
});

fork(function *() {
    console.log('start!');
    yield wait(0.1);
    console.log('expected: 123 ==', yield Promise.resolve(123));

    var loops = [1e5, 2e5, 1e6, 2e6, 1e7];
    for (var j in loops) {
        var time = Date.now();
        for (var i = 0, N = loops[j]; i < N; ++i)
            if (i !== (yield test(i)))
                throw new Error('eh!?');
        var delta = (Date.now() - time) / 1000;
        console.log('%s sec, %s tps', delta.toFixed(3), N / delta);
        yield wait(0.1);
    }

    return 12345;
}()).then(val => console.log('finished:', val), err => console.error('eh!?:', err.stack || err));
C:\xxx>node discover-leak-in-promise.js
start!
expected: 123 == 123
0.447 sec, 223713.64653243846 tps
0.997 sec, 200601.80541624874 tps
4.813 sec, 207770.6212341575 tps

<--- Last few GCs --->

    8559 ms: Scavenge 702.0 (738.6) -> 702.0 (738.6) MB, 12.3 / 0 ms (+ 2.0 ms in 1 steps since last GC) [allocation failure] [incremental marking delaying mark-sweep].
    9355 ms: Mark-sweep 702.0 (738.6) -> 701.9 (738.6) MB, 796.0 / 0 ms (+ 3.0 ms in 2 steps since start of marking, biggest step 2.0 ms) [last resort gc].
   10162 ms: Mark-sweep 701.9 (738.6) -> 701.9 (738.6) MB, 807.6 / 0 ms [last resort gc].


<--- JS stacktrace --->

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

Security context: 28E85C51 <JS Object>
    1: PromiseSet(aka PromiseSet) [native promise.js:~32] [pc=0835AF57] (this=28E080C9 <undefined>,m=28E9DD0D <a Promise with map 3E80BBCD>,p=0,q=28E080C9 <undefined>,r=28E9DD3D <JS Array[0]>,s=28E9DD2D <JS Array[0]>)
    2: PromiseInit(aka PromiseInit) [native promise.js:~47] [pc=08357422] (this=28E080C9 <undefined>,m=28E9DD0D <a Promise with map 3E80BBCD>)
    3: defer [native promise.js:~135] [pc=...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory
@mscdex mscdex added v8 engine Issues and PRs related to the V8 dependency. question Issues that look for answers. labels Dec 9, 2015
@LightSpeedC
Copy link
Author

I think combined code, native Promise and arrow function, cause LEAK BUG.
Why following code 0 LEAK?

LEAK code 0: native Promise and arrow function expression without block statements.

var fork = gen => new Promise((res, rej, cb) => (
    cb = (err, val) => (
        (val = err ? gen.throw(err) : gen.next(val)),
        (val.done ? res(val.value) : val.value.then(val => cb(null, val), cb))
    ), cb()
));

NOT LEAK code 1: arrow function expression with block statements.

var fork = gen => new Promise((res, rej) => {
    var cb = (err, val) => {
        try { val = err ? gen.throw(err) : gen.next(val); } catch (e) { rej(e); }
        val.done ? res(val.value) : val.value.then(val => cb(null, val), cb);
    }; cb();
});

or

NOT LEAK code 2: do not use native Promise. npm "bluebord" or npm "promise-light".

var Promise = require('bluebird');
// or
var Promise = require('promise-light');

or

NOT LEAK code 3: use famous coroutine library, like npm "co". or miner npm "aa".

var fork = require('co');
//
var fork = require('aa');

Why code 0 LEAK?

@dnalborczyk
Copy link
Contributor

I think your leakage has nothing to do with using statements or expressions. I reduced your code to a reproducible minimum with both versions - though I must admit I wasn't quite sure what the code tried to do. Both error out.

Btw, in order to make it (not) work, I had to let .then not return cb anymore - not sure if that was important or not?

var test = val => Promise.resolve( val );

var fork_exp = gen => new Promise( ( res, rej, cb ) => (
    cb = ( err, val ) => (
        val = gen.next( val ),
        val.value.then( val => cb( null, val ) )
    ), cb()
));

var fork_st = gen => new Promise( ( res, rej ) => {
    var cb = ( err, val ) => {
        val = gen.next( val );
        return val.value.then( val => cb( null, val ) )
    };
    cb()
});

var fork = fork_st; // fork_exp

fork( function *() {
    for ( var i = 0;; i++ ) {
        yield test( i );
    }
}());

I believe you are experiencing a V8 Promise bug:

zloirock/core-js#78
#2747

The bug has been fixed recently in v8, but hasn't made its way into Node.js yet.
https://bugs.chromium.org/p/v8/issues/detail?id=4162

In order to (temporary) fix the problem you can use another userland Promise implementation as you suggested:

var bluebird = require( 'bluebird' );
var test = val => bluebird.resolve( val );

PS: Is that a good idea to declare a variable in the function callback for the expression? Couldn't it possibly cause issues if the Promise constructor would extend the callback signature to a third parameter? Not like that they would ever do that, but hypothetically.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Closing as it's not an actionable issue right now.

@jasnell jasnell closed this as completed Mar 22, 2016
@lobabob
Copy link

lobabob commented Dec 21, 2016

Just checking in on the current state of this since it's been a year. Is this something that's actionable now? Has it already been resolved? Am I talking about this in the wrong place?

@joyeecheung
Copy link
Member

As #4210 (comment) indicates the fix landed in V8 master last year so I think it should already landed in Node now. You can try to reproduce it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

6 participants