Fix crashes from template finalizer #182
Conversation
Codecov Report
@@ Coverage Diff @@
## master #182 +/- ##
==========================================
+ Coverage 94.91% 94.96% +0.05%
==========================================
Files 12 12
Lines 492 497 +5
==========================================
+ Hits 467 472 +5
Misses 15 15
Partials 10 10
Continue to review full report at Codecov.
|
dylanahsmith
force-pushed
the
template-safe-finalizer
branch
from
ebac6bf to
cf200ca
Compare
| t.ptr = nil | ||
| runtime.SetFinalizer(t, nil) |
There was a problem hiding this comment.
From https://pkg.go.dev/runtime#SetFinalizer
When the garbage collector finds an unreachable block with an associated finalizer, it clears the association and runs finalizer(obj) in a separate goroutine. This makes obj reachable again, but now without an associated finalizer.
so Go should will clear the finalizer for us, thus we don't need to do that here.
…apper v8::Persistent::Reset() shouldn't be used without synchronization from the finalizer goroutine to avoid race conditions. Instead, we can just free the wrapper and leave the V8 data on the isolate heap to be freed when the isolate is disposed.
dylanahsmith
force-pushed
the
template-safe-finalizer
branch
from
608e79d to
461353d
Compare
Otherwise, the garbage collector can collect the template and the finalizer can free the C++ m_template struct before it is finished being used.
dylanahsmith
force-pushed
the
template-safe-finalizer
branch
from
461353d to
c7c6d8f
Compare
* Fix crash from template finalizer releasing V8 data, just free the wrapper v8::Persistent::Reset() shouldn't be used without synchronization from the finalizer goroutine to avoid race conditions. Instead, we can just free the wrapper and leave the V8 data on the isolate heap to be freed when the isolate is disposed. * Keep alive the template while its C++ pointer is still being used Otherwise, the garbage collector can collect the template and the finalizer can free the C++ m_template struct before it is finished being used.
Problem
We ran into fairly reproducible crashes from benchmarking code that was creating (and initializing) new isolates frequently, often in C.TemplateSetTemplate, where the value being set was a v8go.FunctionTemplate. At first, I thought this might have been from a race condition from the finalizer running in a separate goroutine, which I thought was the reason other uses of finalizers was removed from v8go, but it happened even when the finalizer only freed the wrapper struct (m_template). It looks like there was also a problem with the
templatebecoming garbage aftertemplate.ptris obtained from it, even beforetemplate.ptris passed into the C function (e.g.C.TemplateSetTemplate).Solution
To avoid the finalizer goroutine race condition, I changed the finalizer to only free the template wrapper struct. v8::Template resides in the V8 heap (i.e. it is v8::Data), so it should get freed when the isolate is disposed. In the future, the solution for #105 should allow this v8::Data to be released earlier.
The second commit in this PR uses runtime.KeepAlive to avoid the template Go struct from being garbage collected before the
template.ptris finished being used.I tested this in our application benchmark that was crashing and this seems to have fixed the crashes.