AsyncHookCleanupHandle is not ABI safe across different versions of the std lib

[implausible]

• Version: Any version
• Platform: All platforms
• Subsystem: native modules

What steps will reproduce the bug?

1. Create a native node module that uses NaN and async cleanup behavior via node::AddEnvironmentCleanupHook.
2. Compile for Electron 10.1.4 or higher
3. Failure to build, because std::unique_ptr is not an ABI compatible interface

How often does it reproduce? Is there a required condition?

It reproduces when node is compiled for a different version of C++ than what MSVC uses with node-gyp. It looks like Chromium and therefore Electron switched which c++ stdlib it uses internally in M75.

What is the expected behavior?

Native modules should be able to use this API regardless of whether we're compiling for Electron or Node.

What do you see instead?

node-gyp cannot compile native modules for Electron when referencing a method that uses AsyncCleanupHookHandle, because it is just a typedef of std::unique_ptr.

Additional information

This is an issue with the changes implemented in response to this issue #34715. I think this is just something that was overlooked as when I inspected related code it looks like developers and reviewers were aware of this as an issue in the first place :) See https://github.com/nodejs/node/blob/master/src/api/hooks.cc#L103-L106.

@addaleax Help would be much appreciated :) We were unable to launch NodeGit for Electron 11 because of this :(.

[addaleax]

Do you happen to know how the two ABIs differ?

Either way, I guess one practical approach to this problem would be replacing AsyncCleanupHookHandle with a custom class. I’m not sure how to do it in a way that doesn’t take a while to reach Electron production versions, though.

[implausible]

It looks like the generated symbols for node in electron 10.1.4 reference __1@std in every usage of std, where as the symbols generated by node-gyp do not include __1@std but instead std. I think this is largely because of https://chromium.googlesource.com/chromium/src/+/ff480e1803a097f5485fbee9833edb04e96676e0 where chromium moved to libc++ instead of libstdc++. Are you aware of a way to convince node-gyp to leverage libc++ through msvc instead of libstdc++? That would be sufficient to workaround the issue. I believe that's not possible without a change in compiler, which I'm not well informed enough to know if that's possible on windows with node-gyp.

Just for example, the symbols that are being shipped with node.lib for win-x64 are:
__imp_?AddEnvironmentCleanupHook@node@@YA?AV?$unique_ptr@UACHHandle@node@@UDeleteACHHandle@2@@__1@std@@PEAVIsolate@v8@@P6AXPEAXP6AX1@Z1@Z1@Z
And the symbol node-gyp is looking for is:
__imp_?AddEnvironmentCleanupHook@node@@YA?AV?$unique_ptr@UACHHandle@node@@UDeleteACHHandle@2@@std@@PEAVIsolate@v8@@P6AXPEAXP6AX1@Z1@Z1@Z

[implausible]

I think this difference is primarily because of an inline namespace in libc++ that doesn't exist in libstdc++..

[addaleax]

That sounds like a fair assumption, yes … I’m wondering if this something you might want to report to the Electron team instead/as well? There are other uses of the C++ standard library in Node’s and V8’s APIs as well, which are likely also affected then.

/cc @nodejs/platform-windows

[implausible]

My approach for this was that I was going to reference this particular issue to the Electron team. My thinking now is that this is likely going to be a coordination of both the Node team and the Electron team. Also, with Electron 11 having just landed with such a dramatic change, I think we have time to understand these issues as even with the beta period it can take some time for all of the issues to be discovered. Especially with stable production environments often lagging behind the bleeding edge.

Yikes.. I'm looking at the headers for V8 and you're right, this compiler change is rough. I will include that in my report to Electron.

[implausible]

@addaleax Hope you had a good new year. So I'm back looking at this catastrophe. Hear me out 😄 I went through the uses of stdlib that are exported from node and v8 and there's literally only one of them that is a critical linchpin and that's this API. Honestly every other instance of the stdlib is likely not as critical as this functionality being ABI compatible (and quickly).

I've been digging through the Electron build process to at least have an idea of what it would take someone brilliant enough to make libc++ exported from Electron itself and it's a monumental task. I have no clue when the team would be able to accomodate this issue, nor do I know when I would be able to learn enough to solve it myself.

Is there any chance that we could get rid of the requirement on std::unique_ptr here and instead export a standard pointer that can be deleted manually (or provide a cleanup function for the pointer)? I see the value in using std::unique_ptr here as it is generally the way to handle this pattern in C++, but I also see the need as a consumer of the API to be able to be context-aware early this year. Further, say we did use a less-than-standard API here where we start by releasing this as just the raw pointer... We could leverage NAN to wrap that API in a std::unique_ptr that will function for developers today, and if in the future electron is able to export libc++ in a way that is suitable for native developers... we could reverse this decision and export the std::unique_ptr from the API directly instead of through NAN.

I don't mean to rush this through - but there were some big issues with Electron 11 and Big Sur, one of those being libuv/libuv#3064 which I have no clue how far back this will be backported. If that were to land in 11 we would not be able to build NodeGit for Electron 11 and thus satisfy customer issues. I know this particular argument isn't particularly strong but it scares me a lot.

TLDR:

• Exporting libc++ is a difficult task that may not get done within the year, if ever (because it's generally not a great idea anyway)
• An uglier API here would would open up integrations in a timely manner
• NAN would help keep the api the same as it is currently in a native-module friendly way
• Getting left behind in Electron would be awful

Hope I'm not begging too hard here, thoughts?

[implausible]

Ok I can confirm from compiling Electron once that libc++ isn't even technically a build target (generates no .lib file on Windows). I think it is being linked at the time Electron.exe is built. This makes it harder for me to champion fixing this in Electron, because that's a lot of build system to learn haha. I am going to try playing around with an API that doesn't use std::unique_ptr and see if I can get a native module working with it correctly. If that's the case, I will submit a PR for it and see if we can gain any traction there. I just don't personally anticipate that I'm going to get much traction with trying to export libc++ by myself or through help from smarter people than me.

[addaleax]

@implausible Well … yes, sure, I think that makes sense. We could still use std::unique_ptr here, we just have to make sure that it’s generated inline in the node.h header, not in Node.js’s own C++ files.

There’s also other, unrelated issues with the C++ stdlib ABI that I’ve run into myself, like #36634 and #30786 – ultimately, it seems like providing “raw” interfaces and then wrapping over them in inline headers seems sensible. For our own code, that’s not too hard, it’s only for V8 APIs where we’d need to put in a bit more work, I think.

[implausible]

We could still use std::unique_ptr here, we just have to make sure that it’s generated inline in the node.h header, not in Node.js’s own C++ files.

This is a much better idea than I had 😂. So something like this? implausible@2347de8

I am going to plug this into Electron locally and check that it alleviates at least this issue. Thanks for the help!

[addaleax]

@implausible Yeah, exactly :) Feel free to submit a PR and ping me, if you like

[implausible]

Thanks! I just got everything compiled with Electron and it actually loaded! 😄 So much stress just melted away. I'll get that PR open. (after a quick break for lunch).