Possible memory leak-- processPromiseRejections never called?

[mattkrick]

After migrating from express to uwebsockets, i noticed a memory leak caused by node holding onto unhandled rejections and never flushing them

The pendingUnhandledRejections array is emptied when processPromiseRejections is called: https://github.com/nodejs/node/blob/c9b93e234454322ac0b7a6cd29d394f428f3e37d/lib/internal/process/promises.js#L171

It should get called every tick here: https://github.com/nodejs/node/blob/68abaab8baac203833889c9106abf6fe82a5900f/lib/internal/process/task_queues.js#L98

Perhaps the task queue is never empty, so !queue.isEmpty() || processPromiseRejections() always shortcircuits? Any guidance appreciated

[ghost]

That's probably a correct find, I've been spending far too long trying to fight that whole dreadful nextTick nonsense.

It has been the finger nail in my eye for a long time and I properly hate it for existing.

The only proper way is to use node::MakeCallback which is a massive performance hit since nodejs is a pile of garbage.

In any case, I'll probably have to accept nodejs is nodejs. It's easier to deal with knowing Python has no such nonsense.

[mattkrick]

i hear ya,
dumb idea because i'm not familiar with node internals-- is it possible to just call process.runNextTicks?
Since _tickCallback was deprecated, that looks like the new userland way to handle it.

issue: nodejs/node#29671
old example from node v10: https://blog.insiderattack.net/crossing-the-js-c-boundary-advanced-nodejs-internals-part-1-cb52957758d8

[ghost]

It's an easy fix, all you have to do is resort to the "correct" way of calling in to JavaScript.

Problem is, that "correct" way is really bad, and limits performance a lot.

Vanilla V8 Function calls are fast, I get 123k messages/second while node::MakeCallback is down to 99k. It's a major hit to performance.

However, since I have found Python and already do 137k messages/second there I think it is time to just let nodejs be nodejs like I said earlier, and work more on the Python extension when it comes to showcasing performance.

Moving back to the "correct" way would solve a lot of strange crashes when debugging and just overall make it more robust.

I see it as a speeding ticket, now it's time to pay the price.

[IhsanMujdeci]

Does this mean you will stop developing the js library?

[ghost]

No I'm just saying, writing the Python extension is a pleasure. It's the scripting environment that really lets uWS shine

[ghost]

@mattkrick Please try npm install uNetworking/uWebSockets.js#binaries and see if that solves your problems

[mtsewrs]

@alexhultman why do this if performance is sacrificed? Is this a non issue if you just catch all promises?

[ghost]

Because there are other problems such as the debugger crashing.

[mattkrick]

verified that the leak no longer exists after installing binaries. thank you!

sidenote-- i tried to downgrade to v16.5.0 to reproduce the leak, but kept getting an error during heap dump (/usr/local/bin/node[6355]: ../src/api/callback.cc:110:void node::InternalCallbackScope::Close(): Assertion (env_->execution_async_id()) == (0)' failed.`). blew away the yarn cache, node_modules & lockfile, still couldn't get the heapdump to work for the old version. not a huge deal, just stumped on what could've broken

[ghost]

That crash you got in v16.5 is exactly the kind of issues this commit solves, so it further highlights the need for it

[ghost]

Having debugger, heap dump and this memory leak fixed is probably worth it even though I hate to say it. Great