From 88e5a148ae4b31750a4c86fdccff5f7f80c406f7 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 17 Sep 2024 14:09:32 -0400 Subject: [PATCH 1/2] enforce isolate lock and track allocations on v8 --- src/workerd/api/node/zlib-util.h | 1 - src/workerd/api/streams/compression.c++ | 28 +++++++++++++++++-------- src/workerd/api/streams/compression.h | 15 ++++++------- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/src/workerd/api/node/zlib-util.h b/src/workerd/api/node/zlib-util.h index ed1310f5c13..be7126bcb34 100644 --- a/src/workerd/api/node/zlib-util.h +++ b/src/workerd/api/node/zlib-util.h @@ -293,7 +293,6 @@ class ZlibUtil final: public jsg::Object { class CompressionStream: public jsg::Object { public: explicit CompressionStream(ZlibMode _mode): context_(_mode) {} - CompressionStream() = default; // TODO(soon): Find a way to add noexcept(false) to this destructor. ~CompressionStream(); KJ_DISALLOW_COPY_AND_MOVE(CompressionStream); diff --git a/src/workerd/api/streams/compression.c++ b/src/workerd/api/streams/compression.c++ index e5e8c31c87a..810afd998c3 100644 --- a/src/workerd/api/streams/compression.c++ +++ b/src/workerd/api/streams/compression.c++ @@ -26,20 +26,28 @@ void* CompressionAllocator::AllocForZlib(void* data, uInt items, uInt size) { return AllocForBrotli(data, real_size); } -// TODO(soon): Enabling V8 reporting breaks CompressionStreamImpl on edgeworker. Investigate. -// This is mostly likely due to running CompressionStream without an isolate lock. void* CompressionAllocator::AllocForBrotli(void* opaque, size_t size) { - auto* thisAllocator = static_cast(opaque); - auto data = kj::heapArray(size); + auto* allocator = static_cast(opaque); + auto data = kj::heapArray(size); auto begin = data.begin(); - thisAllocator->allocations.insert(begin, kj::mv(data)); + auto isolate = v8::Isolate::GetCurrent(); + KJ_REQUIRE_NONNULL(isolate, "Isolate lock is required"_kj); + auto& js = jsg::Lock::from(isolate); + allocator->allocations.insert(begin, + { + .data = kj::mv(data), + .memoryAdjustment = js.getExternalMemoryAdjustment(size), + }); return begin; } void CompressionAllocator::FreeForZlib(void* opaque, void* pointer) { if (KJ_UNLIKELY(pointer == nullptr)) return; - auto* thisAllocator = static_cast(opaque); - JSG_REQUIRE(thisAllocator->allocations.erase(pointer), Error, "Zlib allocation should exist"_kj); + auto* allocator = static_cast(opaque); + // No need to destroy memoryAdjustment here. + // Dropping the allocation from the hashmap will defer the adjustment + // until the isolate lock is held. + JSG_REQUIRE(allocator->allocations.erase(pointer), Error, "Zlib allocation should exist"_kj); } namespace { @@ -138,6 +146,9 @@ public: }; } +protected: + CompressionAllocator allocator; + private: static int getWindowBits(kj::StringPtr format) { // We use a windowBits value of 15 combined with the magic value @@ -161,7 +172,6 @@ private: Mode mode; z_stream ctx = {}; kj::byte buffer[16384]; - CompressionAllocator allocator; // For the eponymous compatibility flag ContextFlags strictCompression; @@ -469,7 +479,7 @@ private: }; } // namespace -jsg::Ref CompressionStream::constructor(jsg::Lock& js, kj::String format) { +jsg::Ref CompressionStream::constructor(kj::String format) { JSG_REQUIRE(format == "deflate" || format == "gzip" || format == "deflate-raw", TypeError, "The compression format must be either 'deflate', 'deflate-raw' or 'gzip'."); diff --git a/src/workerd/api/streams/compression.h b/src/workerd/api/streams/compression.h index 13f23dd3503..7172c0762b6 100644 --- a/src/workerd/api/streams/compression.h +++ b/src/workerd/api/streams/compression.h @@ -18,7 +18,6 @@ namespace workerd::api { // isolate pointer and use that to get the external memory adjustment. class CompressionAllocator final { public: - CompressionAllocator() = default; void configure(z_stream* stream); static void* AllocForZlib(void* data, uInt items, uInt size); @@ -26,20 +25,18 @@ class CompressionAllocator final { static void FreeForZlib(void* data, void* pointer); private: - // TODO(soon): Use this instead of array after fixing edgeworker. - // struct Allocation { - // kj::Array data; - // kj::Maybe memoryAdjustment; - // }; - - kj::HashMap> allocations; + struct Allocation { + kj::Array data; + jsg::ExternalMemoryAdjustment memoryAdjustment; + }; + kj::HashMap allocations; }; class CompressionStream: public TransformStream { public: using TransformStream::TransformStream; - static jsg::Ref constructor(jsg::Lock& js, kj::String format); + static jsg::Ref constructor(kj::String format); JSG_RESOURCE_TYPE(CompressionStream) { JSG_INHERIT(TransformStream); From 64ff1c5bbd9ca33c4168701d06bbbc85ce4cab85 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 25 Sep 2024 11:56:11 -0700 Subject: [PATCH 2/2] fixup! enforce isolate lock and track allocations on v8 --- src/workerd/api/streams/compression.c++ | 22 ++++++++++++++++++---- src/workerd/api/streams/compression.h | 2 +- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/workerd/api/streams/compression.c++ b/src/workerd/api/streams/compression.c++ index 810afd998c3..6a0c76a2fa0 100644 --- a/src/workerd/api/streams/compression.c++ +++ b/src/workerd/api/streams/compression.c++ @@ -30,13 +30,27 @@ void* CompressionAllocator::AllocForBrotli(void* opaque, size_t size) { auto* allocator = static_cast(opaque); auto data = kj::heapArray(size); auto begin = data.begin(); - auto isolate = v8::Isolate::GetCurrent(); - KJ_REQUIRE_NONNULL(isolate, "Isolate lock is required"_kj); - auto& js = jsg::Lock::from(isolate); + auto isolate = v8::Isolate::TryGetCurrent(); + kj::Maybe maybeMemoryAdjustment; + // TODO(soon): Improve this. We want to track external memory allocations + // with the v8 isolate so we can account for these as part of the isolate + // heap memory limits. However, we don't always have an isolate lock or + // current isolate when this is called so we can't just blindly try + // grabbing the isolate. For now we'll only be able to account for the + // allocations when we actually have an isolate. It's a bit tricky but + // we could possibly try implementing a deferred accounting adjustment? + // Basically, defer incrementing the memory allocation reported to the + // isolate until we have the isolate lock again? But that's a bit tricky + // if the adjustment is dropped before that happens. Will have to think + // through how best to approach that. + if (isolate != nullptr) { + auto& js = jsg::Lock::from(isolate); + maybeMemoryAdjustment = js.getExternalMemoryAdjustment(size); + } allocator->allocations.insert(begin, { .data = kj::mv(data), - .memoryAdjustment = js.getExternalMemoryAdjustment(size), + .memoryAdjustment = kj::mv(maybeMemoryAdjustment), }); return begin; } diff --git a/src/workerd/api/streams/compression.h b/src/workerd/api/streams/compression.h index 7172c0762b6..fcd374d3db3 100644 --- a/src/workerd/api/streams/compression.h +++ b/src/workerd/api/streams/compression.h @@ -27,7 +27,7 @@ class CompressionAllocator final { private: struct Allocation { kj::Array data; - jsg::ExternalMemoryAdjustment memoryAdjustment; + kj::Maybe memoryAdjustment = kj::none; }; kj::HashMap allocations; };