nodejs / node-addon-api

Module for using Node-API from C++
MIT License
2.17k stars 459 forks source link

A stateless addon blows up after a few thousand calls...? #1577

Closed kamil321 closed 1 month ago

kamil321 commented 1 month ago

Hello there. I'm trying to wrap a relatively simple, stateless parsing library (erengy/anitomy) into node-addon-api. It gets a string and returns a vector, which I'm converting into a Napi::Object: https://github.com/hiijac/anitomy-napi/blob/master/src/main.cpp

Then I'm executing it in a simple loop: https://github.com/hiijac/anitomy-napi/blob/master/test/test.js

It works flawlessly for the first 4592 times, and then pretty weird things start to happen. As far as I understand, an empty vector passed into a lambda suddenly appears as it would have a max possible size instead of 0. This causes emplace_back to try to write to a nullptr and then node just shuts down.

The exception analysis: https://raw.githubusercontent.com/hiijac/anitomy-napi/master/test/exception.txt

My repo: https://github.com/hiijac/anitomy-napi

Reproduction: it requires a c++23 compatible msvc toolchain, but then you can just: npm i && npm run build-debug cd test npm i && npm start Attaching a debugger then would obviously be a good idea.

I'm not even completely sure it's a napi issue, but with a loop on the cpp side everything works ok (tested up to a million iterations). Any help appreciated!

//a small update:

KevinEady commented 1 month ago

Hi @kamil321 , thank you for such a detailed post 🙇

Question: can you test with defining NAPI_EXPERIMENTAL as well? There are some changes to the object finalization on the Node-API side when using experimental mode (mostly in effect when using Finalizers) but it would be a good thing to check.

NickNaso commented 1 month ago

@kamil321 Do I need Windows to test? It seems that it does not build on macOS. Maybe I'm doing something wrong. I cloned you project and then I execute the command npm run build.

kamil321 commented 1 month ago

@KevinEady

so no luck with NAPI_EXPERIMENTAL

@NickNaso What's the error? Tbh I only tested it on windows, as I simply don't have a mac. Anitomy is a bit compiler-fussy, as it uses e.g. std::ranges::views::adjacent from c++23, but other than that - there is no platform dependent code as far as I'm aware. I probably forgot about something in cmake though.

//edit Lint me sideways, I've finally fixed it. Now it works on any node, experimental napi or not, with or without random awaits. How?

There were those 2 suspicious (according to the stack trace) lambdas in episode.hpp capturing a reference to a vector, something like:

std::vector<Element> elements; static const auto add_element = [&elements](ElementKind kind, //....

and this vector's size was somehow incorrectly calculated inside them after all those iterations. Why? Still no idea. But I just converted them into some plain old inline functions, like:

inline void add_element(std::vector<Element>& elements, ElementKind kind, //...

I thought about it earlier, but it was just too preposterous to try out. And yet, it helped.

The bug has fled, the work is done, The code and I, again, are one.

Still, it might be worth investigating why doesn't node's gc / napi like this construction, so I'm here if you need me.

mhdawson commented 1 month ago

Closing out since problem is resolved.

kamil321 commented 1 month ago

@mhdawson resolved with a workaround to be precise, as the original code works fine without napi, but ayw, I'll live.