Conversation
774421e to
4677f54
Compare
|
Same comment as #21 -- please write prose intended for humans (commit messages, PR messages, code comments) by hand. |
Don't derive CtxWrap from node::ObjectWrap as it has a known bug in interaction with GC when numerous instances (>1000) are created and aborts the process during isolate teardown. This was historically not much of an issue when only few instances were created by add-ons but with the advent of AsyncContextFrame now we can indeed have thousands of objects being created.
4677f54 to
5281a2b
Compare
|
@umanwizard rewrote PR description and commit messages by hand here as well. (Also significantly reduced the amount of in-code comments.) |
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
I've given it a pass! It does look reasonable, but it's probably worth having a second set of eyes on this since my C++/Node-fu is not amazing.
| // Spawned by the "contexts collected during isolate teardown" test. Runs in | ||
| // its own process because the failure mode is a SIGABRT, which would take the | ||
| // whole test run down with it. | ||
| // | ||
| // When CtxWrap derived from node::ObjectWrap, a CtxWrap collected during | ||
| // isolate teardown ran ~ObjectWrap -> RemoveEnvironmentCleanupHook, which | ||
| // CHECKs that an Environment is current. It is not, during teardown, so: | ||
| // | ||
| // Assertion failed: (env) != nullptr | ||
| // 3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap() | ||
| // | ||
| // It needs enough instances (~1000) that V8 still has some left to collect | ||
| // at teardown. |
There was a problem hiding this comment.
It may be just me, but this reads a bit confusing.
Looking at the high-level, this is a regression test for the previous implementation detail where CtxWrap extended from ObjectWrap -- this test would crash prior to the other changes in the PR, and now it doesn't.
But the description gets a bit too much into detail and at least for me the above bit -- which IMHO is the only important part here -- is not clear.
(In the context of the PR it's obvious what this does, but if I were looking at just this file without prior context I'm not sure I'd understand what's going on/what's being tested here)
Also, the whole harness introduced in
Runs in its own process because the failure mode is a SIGABRT, which would take the whole test run down with it.
I think is not very valuable anymore? E.g. we could keep it around as a previous commit in the branch, but going forward the issue is expected to be fixed forever, so why pay the cost of a spawn and additional complexity for a test that's never expected to ever fail ever again?
There was a problem hiding this comment.
That's kinda typical for me, I like putting reproducers into issues as a first commit, so a reviewer can check out the repo at that commit and see evidence that the issue is real, and then they can check out the next commit, and see that it fixes the issue.
It's true that this should not occur again, though. I'm happy to remove it or… how about this: I'll add a commit that reverts this commit after the fix commit, so the reproducer stays here in the PR branch, but vanishes from the squashed commit when we merge.
There was a problem hiding this comment.
Ah, to be clear, my comment was more along the lines of:
- It's not clear (from the comments) that the test was a reproducer
- Since we don't expect the issue to happen again, I think the whole custom harness seems a bit overkill; maybe worth instead keeping around only the smallest thing that reproduces the crash (even though it might not be the more "ergonomic" one, since it crashes the whole node process doing the tests)
| // Layout note for the reader: `record_` is private to C++ but its byte | ||
| // position within CtxWrap is part of the reader contract. It is the first | ||
| // field after the node::ObjectWrap base subobject. `capacity_` sits after | ||
| // field of the class, at offset zero. `capacity_` sits after | ||
| // `record_` purely for the writer's own bookkeeping — the reader never | ||
| // touches it. |
There was a problem hiding this comment.
Minor: Should we maybe mention "Use native_wrap_fields_offset, don't assume" instead of hardcoding here the value as a comment?
There was a problem hiding this comment.
We can. This offset will actually go away if I implement that follow-up that eliminates one level of indirection. I'll touch up the comment for now regardless.
| // JSObject's internal field 0. With no base class it is simply the first | ||
| // member, so the offset is zero and the published | ||
| // `threadlocal.native_wrap_fields_offset` is computed from this. | ||
| static_assert(std::is_standard_layout<CtxWrap>::value, | ||
| "CtxWrap must stay standard-layout: the reader contract depends " | ||
| "on offsetof(record_) being well-defined"); | ||
| static_assert(offsetof(CtxWrap, record_) == 0, | ||
| "record_ must be the first field of CtxWrap"); |
There was a problem hiding this comment.
Minor: I suggest moving this together with NATIVE_WRAP_FIELDS_OFFSET and avoid all the repeating of details in comments.
IMHO it's a bit redundant to have "0" and "zero" in both code and comments + all these things need to be changed together so it's maybe nice to have them next to each other.
| p->next_ = nullptr; | ||
| // Clear the holder's internal field before freeing what it points at, so | ||
| // nothing can reach a dangling CtxWrap through it — including the | ||
| // out-of-process reader, which walks exactly this slot. Being on the live |
There was a problem hiding this comment.
Wait, is this true -- will the out-of-process reader need to walk the linked list? I thought this change was only related to resource cleanup?
There was a problem hiding this comment.
No it doesn't need to walk this linked list, it's a purely internal construct. The reader walks this slot for exactly one object during any particular lookup. I should replace the word "walks" with "reads"
There was a problem hiding this comment.
Ah, I guess it reads only whatever's the first one, rather than walking the list?
There was a problem hiding this comment.
the out-of-process reader never sees this list. The reader will start from a thread local, find the async context map in the CPED, find the value in the map that the writer sets (which is a JS object), and then read that object's internal field slot to access the CtxWrap object. So the reader traverses this slot for the object that is the value for the currently active async context.
The list on the other hand primarily exists for freeing the memory of native wrappers at runtime destruction – it contains all the live native wrappers. As it's freeing them it also zeroes out the internal field slot of the JS objects wrapping them, as that slot points to the native objects being freed. That's all this code does. I'll reword the comment.
There was a problem hiding this comment.
Your description matches what I expected coming into the PR -- I guess the comment was what confused me.
The latest version still mentions the "out-of-process" reader in relation to maintaining this list, which seems to not match what you describe above -- perhaps consider removing the comment?
There was a problem hiding this comment.
Okay, I'll make another attempt at explaining. I think the comment is now right. Maybe let's visualize this in two dimensions:
On teardown, DrainLiveCtxWraps walks the linked list of CtxWrap objects horizontally starting from g_live_ctx_wraps (bottom of the picture) and performs all kinds of destruction operations. One of those is reaching back to the JSObject mirror of each CtxWrap in the list through its handle_ and setting its pointer to CtxWrap (drawn as a red arrow) to nullptr.
An external reader OTOH reads vertically in the attached picture. It starts from otel_thread_ctx_nodejs_v1.cped_slot, finds an AsyncContextFrame there – the one corresponding to the currently active async context on the thread – and then follows some pointers (details omitted) until it hits the JSObject, then it follows the pointer stored in its internal field to CtxWrap. By zeroing out this pointer in DrainLiveCtxWraps we prevent the reader from trying to read freed memory (as CtxWrap will be deleted immediately after this pointer is zeroed.) Readers being eBPF it's not particularly harmful for them to follow a dangling pointer, but it's nicer to not leave them around.
As an aside: I put in more AsyncContextFrames on the picture right hand side to indicate that there is a bunch of them alive at any given time, and through each of them there's a particular JsObject/CtxWrap pair being accessible (it's possible for the same pair to be reachable from multiple AsyncContextFrames if they're all continuations of each other, or in other words AsyncContextFrame:CtxWrap is N:1.)
So: DrainLiveCtxWraps walks the list (horizontally) and among other things deletes pointers that a reader might be following (vertically) to the objects now being deleted. I tried to express this with the comment, but if the comment is indeed still confusing, I'm happy to remove it. Too bad I can't just put this diagram in the comment 😁.
There was a problem hiding this comment.
AAAAAAAAAAAH I totally get it now. Thanks for going to the trouble to throughly explain, it's crystal clear now.
I'd read through this PR and earlier PRs and was aware of the "vertical" traversal and then this PR added the "horizontal" and I wasn't connecting the dots on how both traversals were... (ahem) connected.
| // Deliberately not a node::ObjectWrap as it has a known bug in interaction | ||
| // with GC when numerous instances are created and can abort the process during | ||
| // isolate teardown. Instances live at shutdown are deleted using DrainLiveCtxWraps. |
There was a problem hiding this comment.
Can we link to the Node issue?
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
👍 This looks great, thanks for all the extra explanations 🙏. Always learning more cool stuff :D
3 participants
This PR rewrites
CtxWrapto not inherit fromnode::ObjectWrap, as it has a known bug in interaction with GC when numerous instances (>1000) are created and aborts the process during isolate teardown. This was historically not much of an issue when only few instances were created by add-ons but with the advent of AsyncContextFrame now we can indeed have thousands of objects being created.Here's a list of existing Node.js issues and PRs regarding this problem for reference:
node::ObjectWrapnodejs/node#63642CleanupHookThunkRunfor everynode::ObjectWrapalive at teardown nodejs/node#65195The problem is that Node.js committers will likely fix this for 26.3 and maybe backport to 24 but 22, 23, and 25 remain vulnerable, so our fix needs to be a permanent one and not just a workaround. Our solution here is to add an intrusive linked list to the
CtxWraptype so we can have a linked list of all live instances rooted in a thread local, and a teardown function invoked from a single environment cleanup hook that walks the list and destroys the instances.Our change is, I believe, materially better than the Node.js
ObjectWrapfix as it buys usrecord_at offset 0 and one cleanup hook per isolate rather than one per instance, independently of any Node fix.BTW, this does affect
CtxWrapon the main branch as well, although it's not in my scope to fix that.For an example of a pretty big win with this approach see this follow-up where we eliminate
CtxWrapas a publicly visible element and eliminate one pointer indirection. I can't stack PRs across repos, so I'm not opening a PR before this lands though.