Skip to content

Commit

Permalink
src: close HandleWraps instead of deleting them in OnGCCollect()
Browse files Browse the repository at this point in the history
When all strong `BaseObjectPtr`s to a `HandleWrap` are gone, we
should not delete the `HandleWrap` outright, but instead close
it and then delete it only once the libuv close callback has been
called.

Based on the valgrind output from the issue below, this has a
good chance of fixing it.

Fixes: #39036

PR-URL: #39441
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and richardlau committed Jul 29, 2021
1 parent 44dba09 commit 55cc488
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/base_object-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ void BaseObject::decrease_refcount() {
unsigned int new_refcount = --metadata->strong_ptr_count;
if (new_refcount == 0) {
if (metadata->is_detached) {
delete this;
OnGCCollect();
} else if (metadata->wants_weak_jsobj && !persistent_handle_.IsEmpty()) {
MakeWeak();
}
Expand Down
11 changes: 10 additions & 1 deletion src/handle_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,16 @@ void HandleWrap::Close(Local<Value> close_callback) {


void HandleWrap::OnGCCollect() {
Close();
// When all references to a HandleWrap are lost and the object is supposed to
// be destroyed, we first call Close() to clean up the underlying libuv
// handle. The OnClose callback then acquires and destroys another reference
// to that object, and when that reference is lost, we perform the default
// action (i.e. destroying `this`).
if (state_ != kClosed) {
Close();
} else {
BaseObject::OnGCCollect();
}
}


Expand Down

0 comments on commit 55cc488

Please sign in to comment.