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

async_hooks: avoid resource reuse by FileHandle #31972

Closed
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
6 changes: 1 addition & 5 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ AsyncWrap::AsyncWrap(Environment* env,
provider_type_ = provider;

// Use AsyncReset() call to execute the init() callbacks.
AsyncReset(execution_async_id, silent);
AsyncReset(object, execution_async_id, silent);
init_hook_ran_ = true;
}

Expand Down Expand Up @@ -653,10 +653,6 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
env->destroy_async_id_list()->push_back(async_id);
}

void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
AsyncReset(object(), execution_async_id, silent);
}

// Generalized call for both the constructor and for handles that are pooled
// and reused over their lifetime. This way a new uid can be assigned when
// the resource is pulled out of the pool and put back into use.
Expand Down
3 changes: 0 additions & 3 deletions src/async_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,6 @@ class AsyncWrap : public BaseObject {
double execution_async_id = kInvalidAsyncId,
bool silent = false);

void AsyncReset(double execution_async_id = kInvalidAsyncId,
bool silent = false);

// Only call these within a valid HandleScope.
v8::MaybeLocal<v8::Value> MakeCallback(const v8::Local<v8::Function> cb,
int argc,
Expand Down
7 changes: 6 additions & 1 deletion src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,12 @@ int FileHandle::ReadStart() {
if (freelist.size() > 0) {
read_wrap = std::move(freelist.back());
freelist.pop_back();
read_wrap->AsyncReset();
// Use a fresh async resource.
// Lifetime is ensured via AsyncWrap::resource_.
Local<Object> resource = Object::New(env()->isolate());
resource->Set(
env()->context(), env()->handle_string(), read_wrap->object());
read_wrap->AsyncReset(resource);
read_wrap->file_handle_ = this;
} else {
Local<Object> wrap_obj;
Expand Down
65 changes: 65 additions & 0 deletions test/async-hooks/test-filehandle-no-reuse.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
'use strict';

const common = require('../common');
const initHooks = require('./init-hooks');
const { checkInvocations } = require('./hook-checks');
const fixtures = require('../common/fixtures');
if (!common.hasCrypto)
common.skip('missing crypto');
const http2 = require('http2');
const assert = require('assert');
const fs = require('fs');

// Checks that the async resource is not reused by FileHandle.
// Test is based on parallel\test-http2-respond-file-fd.js.

const hooks = initHooks();
hooks.enable();

const {
HTTP2_HEADER_CONTENT_TYPE
} = http2.constants;

// Use large fixture to get several file operations.
const fname = fixtures.path('person-large.jpg');
const fd = fs.openSync(fname, 'r');

const server = http2.createServer();
server.on('stream', (stream) => {
stream.respondWithFD(fd, {
[HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
});
});
server.on('close', common.mustCall(() => fs.closeSync(fd)));
server.listen(0, () => {
const client = http2.connect(`http://localhost:${server.address().port}`);
const req = client.request();

req.on('response', common.mustCall());
req.on('data', () => {});
req.on('end', common.mustCall(() => {
client.close();
server.close();
}));
req.end();
});

process.on('exit', onExit);

function onExit() {
hooks.disable();
hooks.sanityCheck();
const activities = hooks.activities;

// Verify both invocations
const fsReqs = activities.filter((x) => x.type === 'FSREQCALLBACK');
assert.ok(fsReqs.length >= 2);

checkInvocations(fsReqs[0], { init: 1, destroy: 1 }, 'when process exits');
checkInvocations(fsReqs[1], { init: 1, destroy: 1 }, 'when process exits');

// Verify reuse handle has been wrapped
assert.ok(fsReqs[0].handle !== fsReqs[1].handle, 'Resource reused');
assert.ok(fsReqs[0].handle === fsReqs[1].handle.handle,
'Resource not wrapped correctly');
}