0

I can see in Chrome task manager that the tab in which following code is running eats more and more memory, and it is not released until the promise is resolved

UPDATE

Main idea here is to use a single 'low level' method which would handle "busy" responses from the server. Other methods just pass url path with request data to it and awaiting for a valuable response.

Some anti-patterns was removed.

var counter = 1

// emulates post requests sent with ... axios
async function post (path, data) {
    let response = (counter++ < 1000) ? { busy: true } : { balance: 3000 }
    return Promise.resolve(response)
}

async function _call (path, data, resolve) {
    let response = await post()

    if (response.busy) {
        setTimeout(() => {
            _call(path, data, resolve)
        }, 10)
        throw new Error('busy')
    }

    resolve(response.balance)
}

async function makePayment (amount) {
    return new Promise((resolve, reject) => {
        _call('/payment/create', {amount}, resolve)
    })
}

async function getBalance () {
    return new Promise((resolve, reject) => {
        _call('/balance', null, resolve)
    })
}

makePayment(500)
    .then(() => {
        getBalance()
            .then(balance => console.log('balance: ', balance))
            .catch(e => console.error('some err: ', e))
    })
humkins
  • 9,635
  • 11
  • 57
  • 75
  • 2
    [Don't use `return await`](https://stackoverflow.com/q/43353087/1048572), avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it), don't pass `resolve` around, promisify at the smallest level (`setTimeout`), and don't throw errors that are never caught! – Bergi Feb 14 '18 at 21:57
  • 3
    not sure about memory, but that code has almost every promise anti-pattern there is :p – Jaromanda X Feb 14 '18 at 21:58
  • Why are you passing a string into `_call`? – Bergi Feb 14 '18 at 21:59
  • The question is, does it release the memory when the `Promise` is not resolved, i.e.: when `Error('busy')` was thrown? If it does, then looks like it work as it supposed to be, no memory leak there. – DJ. Feb 14 '18 at 22:00
  • 1
    This code should amass "unhandled promise rejection" errors, but it should not leak any memory of itself. – Bergi Feb 14 '18 at 22:05
  • 1
    Calling yourself over and over again on a 10ms timer may not be giving the garbage collector enough time to do its normal job. That could be partly why you see memory going up and up until the whole process is done. – jfriend00 Feb 14 '18 at 22:56
  • Plus, this looks like pseudo-code or do-nothing sample code (the function `post()` has no reason to be async or return a promise) which makes it hard to suggest real solutions to a real problem. Honestly, we do a lot better with concrete suggestions when there's real code and a real problem to solve. – jfriend00 Feb 14 '18 at 22:59
  • 1
    FYI, here's a cleaned up version of your code that removes promise anti-patterns: https://jsfiddle.net/jfriend00/9349nuLs/ – jfriend00 Feb 14 '18 at 23:59
  • @Bergi Hello, thank you for your suggestions. Though I know about Promise constructor anti-pattern I don't know how to avoid using it in my example. I've amended my code and added some explanation what string argument of `_call` method is used for. BTW, throw error is an alternative for Promise.reject, isn't it? – humkins Feb 15 '18 at 14:01
  • @jfriend00 Hello, I've updated my code with the problem description. Now it is little more real. – humkins Feb 15 '18 at 14:21
  • 1
    Did you look at the simplified code I linked in my previous comment? It works when I run it in node.js. This is just way more complicated than it needs to be and part of that complication is why you are having problems with it. For example, there's no reason that `post()` should be `async` or should return a promise. It's a simple synchronous function. – jfriend00 Feb 15 '18 at 14:36
  • @jfriend00 Yes, I've seen it. Moreover, I've already applied it to my project. Thank you! – humkins Feb 15 '18 at 18:18

1 Answers1

1

The first time you call _call() in here:

async function getBalance () {
    return new Promise((resolve, reject) => {
        _call('/balance', null, resolve)
    })
}

It will not call the resolve callback and it will return a rejected promise and thus the new Promise() you have in getBalance() will just do nothing initially. Remember, since _call is marked async, when you throw, that is caught and turned into a rejected promise.

When the timer fires, it will call resolve() and that will resolve the getBalance() promise, but it will not have a value and thus you don't get your balance. By the time you do eventually call resolve(response.balance), you've already called that resolve() function so the promise it belongs to is latched and won't change its value.


As others have said, there are all sorts of things wrong with this code (lots of anti-patterns). Here's a simplified version that works when I run it in node.js or in the snippet here in the answer:

function delay(t, val) {
    return new Promise(resolve => {
        setTimeout(resolve.bind(null, val), t);
    });
}


var counter = 1;

function post() {
    console.log(`counter = ${counter}`);
    // modified counter value to 100 for demo purposes here
    return (counter++ < 100) ? { busy: true } : { balance: 3000 };
}

function getBalance () {

    async function _call() {
        let response = post();

        if (response.busy) {
            // delay, then chain next call
            await delay(10);
            return _call();
        } else {
            return response.balance;
        }
    }

    // start the whole process
    return _call();
}

getBalance()
    .then(balance => console.log('balance: ', balance))
    .catch(e => console.error('some err: ', e))
jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • 1
    This is all true (though the `async`/`await` code could be further simplified to use a loop instead of recursion), but it doesn't explain why the OP experienced a memory leak? – Bergi Feb 15 '18 at 19:44