Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

n-api: defer Buffer finalizer with SetImmediate #28082

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,11 @@ void Environment::RunAndClearNativeImmediates() {
if (!try_catch.HasTerminated())
FatalException(isolate(), try_catch);

// We are done with the current callback. Increase the counter so that
// the steps below make everything *after* the current item part of
// the new list.
it++;

// Bail out, remove the already executed callbacks from list
// and set up a new TryCatch for the other pending callbacks.
std::move_backward(it, list.end(), list.begin() + (list.end() - it));
Expand Down
29 changes: 19 additions & 10 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,30 @@ namespace v8impl {

namespace {

class BufferFinalizer: private Finalizer {
class BufferFinalizer : private Finalizer {
public:
// node::Buffer::FreeCallback
static void FinalizeBufferCallback(char* data, void* hint) {
BufferFinalizer* finalizer = static_cast<BufferFinalizer*>(hint);
if (finalizer->_finalize_callback != nullptr) {
NapiCallIntoModuleThrow(finalizer->_env, [&]() {
finalizer->_finalize_callback(
finalizer->_env,
data,
finalizer->_finalize_hint);
});
}
finalizer->_finalize_data = data;
static_cast<node_napi_env>(finalizer->_env)->node_env()
->SetImmediate([](node::Environment* env, void* hint) {
BufferFinalizer* finalizer = static_cast<BufferFinalizer*>(hint);

if (finalizer->_finalize_callback != nullptr) {
v8::HandleScope handle_scope(finalizer->_env->isolate);
v8::Context::Scope context_scope(finalizer->_env->context());

NapiCallIntoModuleThrow(finalizer->_env, [&]() {
finalizer->_finalize_callback(
finalizer->_env,
finalizer->_finalize_data,
finalizer->_finalize_hint);
});
}

Delete(finalizer);
Delete(finalizer);
}, hint);
}
};

Expand Down
33 changes: 20 additions & 13 deletions test/node-api/test_buffer/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,25 @@
const common = require('../../common');
const binding = require(`./build/${common.buildType}/test_buffer`);
const assert = require('assert');
const setImmediatePromise = require('util').promisify(setImmediate);

assert.strictEqual(binding.newBuffer().toString(), binding.theText);
assert.strictEqual(binding.newExternalBuffer().toString(), binding.theText);
console.log('gc1');
global.gc();
assert.strictEqual(binding.getDeleterCallCount(), 1);
assert.strictEqual(binding.copyBuffer().toString(), binding.theText);
(async function() {
assert.strictEqual(binding.newBuffer().toString(), binding.theText);
assert.strictEqual(binding.newExternalBuffer().toString(), binding.theText);
console.log('gc1');
global.gc();
assert.strictEqual(binding.getDeleterCallCount(), 0);
await setImmediatePromise();
assert.strictEqual(binding.getDeleterCallCount(), 1);
assert.strictEqual(binding.copyBuffer().toString(), binding.theText);

let buffer = binding.staticBuffer();
assert.strictEqual(binding.bufferHasInstance(buffer), true);
assert.strictEqual(binding.bufferInfo(buffer), true);
buffer = null;
global.gc();
console.log('gc2');
assert.strictEqual(binding.getDeleterCallCount(), 2);
let buffer = binding.staticBuffer();
assert.strictEqual(binding.bufferHasInstance(buffer), true);
assert.strictEqual(binding.bufferInfo(buffer), true);
buffer = null;
global.gc();
assert.strictEqual(binding.getDeleterCallCount(), 1);
await setImmediatePromise();
console.log('gc2');
assert.strictEqual(binding.getDeleterCallCount(), 2);
})().then(common.mustCall());
5 changes: 4 additions & 1 deletion test/node-api/test_exception/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ const test_exception = require(`./build/${common.buildType}/test_exception`);
function testFinalize(binding) {
let x = test_exception[binding]();
x = null;
assert.throws(() => { global.gc(); }, /Error during Finalize/);
global.gc();
process.on('uncaughtException', (err) => {
assert.strictEqual(err.message, 'Error during Finalize');
});

// To assuage the linter's concerns.
(function() {})(x);
Expand Down