src: warn about FastOneByteString invalidation #59275
Conversation
Minor warning about the use of FastOneByteString.
nodejs-github-bot
added
buffer
| // and produce garbage data. This is not a problem here because we | ||
| // are not performing any allocations or other operations that would | ||
| // trigger a GC before the FastOneByteString is used. Take care when | ||
| // modifying this code to ensure that no operations would trigger a GC. |
There was a problem hiding this comment.
Can we do something like DebugSealHandleScope, i.e. actually enforce this constraint through code? E.g. a DebugDisallowJavascriptExecutionScope that defers to V8's DisallowJavascriptExecutionScope? (That may not be 100% equivalent to disallowing GC, but maybe it's close enough?)
There was a problem hiding this comment.
Possibly? My knowledge of DebugSealHandleScope is super rusty tho so I'm not entirely sure.
There was a problem hiding this comment.
My knowledge of
DebugSealHandleScopeis super rusty tho so I'm not entirely sure.
The point is just that it's possible to at least partially implement something that mechanically prevents GC execution, which is typically better than a comment 🙂 The analogy to DebugSealHandleScope would be that if an explicit DisallowJavascriptExecutionScope is too expensive (it might be?), we could create a debug-only variant of it similar to how DebugSealHandleScope is the debug-only variant of SealHandleScope.
There was a problem hiding this comment.
Yeah, it's a great idea... just would take me a bit of time to page back in the knowledge to do that ;-)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59275 +/- ##
==========================================
+ Coverage 89.96% 89.98% +0.01%
==========================================
Files 649 649
Lines 192141 192141
Branches 37653 37649 -4
==========================================
+ Hits 172867 172903 +36
+ Misses 11883 11835 -48
- Partials 7391 7403 +12
🚀 New features to boost your workflow:
|
|
This could probably do with a Chromium issue as well – FastOneByteString dates from when the fast API could not access the isolate or trigger GC, so presumably this is an oversight? |
Minor warning about the use of FastOneByteString.
Refs: cloudflare/workerd#4625