src: prevent persistent handle resource leaks#18656
Merged
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nodejs-github-bot added c++ 
addaleax approved these changes Feb 8, 2018
joyeecheung approved these changes Feb 8, 2018
src/node_file.cc Outdated
Member
There was a problem hiding this comment.
Maybe just req_wrap->object() here?
MemberAuthor
There was a problem hiding this comment.
Good point, don't know what I was thinking.
cjihrig approved these changes Feb 9, 2018
jasnell approved these changes Feb 9, 2018
jasnell reviewed Feb 9, 2018
src/node_persistent.h Outdated
Member
There was a problem hiding this comment.
A key reason leaks end up happening around all this is the fact that much of this is horribly under-documented. Mind adding some code comments in here that explains how/why this is here?
3 tasks
bnoordhuisforce-pushed the node-persistent branch from 77737b5 to 614fee1CompareMemberAuthor
bnoordhuis commented Feb 11, 2018
MemberAuthor
bnoordhuis commented Feb 12, 2018
parallel/test-http2-createwritereq crashes on some buildbots; the other instances of red are infrastructure issues. Unfortunately, I can't reproduce the failure locally. |
Member
jasnell commented Feb 12, 2018
The http2-createwritereq test does seem to have become a bit flaky lately. Will investigate. /cc @addaleax |
Member
addaleax commented Feb 13, 2018
I can, it fails about 1/4 of the time; I’ll take a look today or tomorrow. Here’s the stack trace: Thread 1"node_g" received signal SIGSEGV, Segmentation fault. 0x0000558f7386c0f9 in v8::base::Relaxed_Load (ptr=0x6057ca10) at ../deps/v8/src/base/atomicops_internals_portable.h:168 168 return __atomic_load_n(ptr, __ATOMIC_RELAXED); (gdb) bt #00x0000558f7386c0f9 in v8::base::Relaxed_Load (ptr=0x6057ca10) at ../deps/v8/src/base/atomicops_internals_portable.h:168 #10x0000558f73888eb7 in v8::internal::HeapObject::map_word (this=0x6057ca11) at ../deps/v8/src/objects-inl.h:1040 #20x0000558f73888e65 in v8::internal::HeapObject::map (this=0x6057ca11) at ../deps/v8/src/objects-inl.h:976 #30x0000558f738885ac in v8::internal::HeapObject::IsJSReceiver (this=0x6057ca11) at ../deps/v8/src/objects-inl.h:295 #40x0000558f73888224 in v8::internal::Object::IsJSReceiver (this=0x6057ca11) at ../deps/v8/src/objects-inl.h:174 #50x0000558f7388577a in v8::Utils::OpenHandle (that=0x558f7706d720, allow_empty_handle=false) at ../deps/v8/src/api.h:353 #60x0000558f738cac2e in v8::Object::InternalFieldCount (this=0x558f7706d720) at ../deps/v8/src/api.cc:6257 #70x0000558f735b17e4 in node::Wrap<void> (object=..., pointer=0x0) at ../src/util-inl.h:223 #80x0000558f735b064e in node::ClearWrap (object=...) at ../src/util-inl.h:228 #90x0000558f7362f50f in node::http2::Http2Session::~Http2Session (this=0x558f77145fe0, __in_chrg=<optimized out>) at ../src/node_http2.cc:535 #100x0000558f7362f5d0 in node::http2::Http2Session::~Http2Session (this=0x558f77145fe0, __in_chrg=<optimized out>) at ../src/node_http2.cc:538 #110x0000558f7364e1c2 in node::BaseObject::WeakCallback<node::http2::Http2Session> (data=...) at ../src/base_object-inl.h:63 #120x0000558f73f34f0d in v8::internal::GlobalHandles::PendingPhantomCallback::Invoke (this=0x7fff11532300, isolate=0x558f770027b0) at ../deps/v8/src/global-handles.cc:859 #130x0000558f73f34c8e in v8::internal::GlobalHandles::DispatchPendingPhantomCallbacks (this=0x558f770226e0, synchronous_second_pass=true) at ../deps/v8/src/global-handles.cc:824 #140x0000558f73f35009 in v8::internal::GlobalHandles::PostGarbageCollectionProcessing (this=0x558f770226e0, collector=v8::internal::MARK_COMPACTOR, gc_callback_flags=v8::kGCCallbackFlagForced) at ../deps/v8/src/global-handles.cc:880 #150x0000558f73f5eeb1 in v8::internal::Heap::PerformGarbageCollection (this=0x558f770027d0, collector=v8::internal::MARK_COMPACTOR, gc_callback_flags=v8::kGCCallbackFlagForced) at ../deps/v8/src/heap/heap.cc:1636 #160x0000558f73f5d57d in v8::internal::Heap::CollectGarbage (this=0x558f770027d0, space=v8::internal::OLD_SPACE, gc_reason=v8::internal::GarbageCollectionReason::kTesting, gc_callback_flags=v8::kGCCallbackFlagForced) at ../deps/v8/src/heap/heap.cc:1257 #170x0000558f73f5ce85 in v8::internal::Heap::CollectAllGarbage (this=0x558f770027d0, flags=2, gc_reason=v8::internal::GarbageCollectionReason::kTesting, gc_callback_flags=v8::kGCCallbackFlagForced) at ../deps/v8/src/heap/heap.cc:1112 #180x0000558f738daa9d in v8::Isolate::RequestGarbageCollectionForTesting (this=0x558f770027b0, type=v8::Isolate::kFullGarbageCollection) at ../deps/v8/src/api.cc:8540 #190x0000558f73ee1d33 in v8::internal::GCExtension::GC (args=...) at ../deps/v8/src/extensions/gc-extension.cc:20 #200x0000558f73906534 in v8::internal::FunctionCallbackArguments::Call (this=0x7fff11532800, f=0x558f73ee1c8e <v8::internal::GCExtension::GC(v8::FunctionCallbackInfo<v8::Value> const&)>) at ../deps/v8/src/api-arguments.cc:25 #210x0000558f739c4d96 in v8::internal::(anonymous namespace)::HandleApiCallHelper<false> (isolate=0x558f770027b0, function=..., new_target=..., fun_data=..., receiver=..., args=...) at ../deps/v8/src/builtins/builtins-api.cc:112 #220x0000558f739c2dc9 in v8::internal::Builtin_Impl_HandleApiCall (args=..., isolate=0x558f770027b0) at ../deps/v8/src/builtins/builtins-api.cc:142 #230x0000558f739c2b63 in v8::internal::Builtin_HandleApiCall (args_length=5, args_object=0x7fff115329e8, isolate=0x558f770027b0) at ../deps/v8/src/builtins/builtins-api.cc:130It sounds a lot like that’s #17840 resurfacing? |
Member
addaleax commented Feb 13, 2018
I’m sorry, I don’t think I’ll able to look into the failures until later this week. @bnoordhuis Would you be okay with me landing #18676 before this one does? Resolving the issue you pointed out over there should be easy enough, and if you want I can do that too. |
MemberAuthor
bnoordhuis commented Feb 13, 2018
No problem, go merge. I'll rebase. |
bnoordhuisforce-pushed the node-persistent branch from 614fee1 to d1ac87bCompareMemberAuthor
bnoordhuis commented Feb 19, 2018
Forgot to push the fixes... CI: https://ci.nodejs.org/job/node-test-pull-request/13271/ Probably needs another review, the fixes are d1ac87b and f0a3019. |
joyeecheung approved these changes Feb 20, 2018
Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
bnoordhuisforce-pushed the node-persistent branch from d1ac87b to 6bdc18cCompareMemberAuthor
bnoordhuis commented Feb 21, 2018
Landed in e53275d...6bdc18c. Infrastructure issue on one of the ARM buildbots, otherwise green:
I'm going to guess someone did an upgrade without rebooting. |
Contributor
MylesBorins commented Feb 21, 2018
Should this be backported to |
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 6, 2018
Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 6, 2018
The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 6, 2018
Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Merged
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Contributor
MylesBorins commented Aug 7, 2018
Should this be backported to |
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace v8::Persistent with node::Persistent, a specialization that
resets the persistent handle on destruction. Prevents accidental
resource leaks when forgetting to call .Reset() manually.
I'm fairly confident this commit fixes a number of resource leaks that
have gone undiagnosed so far.
Related to #18650.