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: merge resource_symbol with owner_symbol #38468

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
2 changes: 1 addition & 1 deletion lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ function asyncResetHandle(socket) {
const handle = socket._handle;
if (handle && typeof handle.asyncReset === 'function') {
// Assign the handle a new asyncId and run any destroy()/init() hooks.
handle.asyncReset(new ReusedHandle(handle.getProviderType(), handle));
handle.asyncReset(new ReusedHandle(handle.getProviderType(), socket));
socket[async_id_symbol] = handle.getAsyncId();
}
}
Expand Down
12 changes: 7 additions & 5 deletions lib/internal/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ const active_hooks = {

const { registerDestroyHook } = async_wrap;
const { enqueueMicrotask } = internalBinding('task_queue');
const { resource_symbol, owner_symbol } = internalBinding('symbols');
addaleax marked this conversation as resolved.
Show resolved Hide resolved
const { owner_symbol } = internalBinding('symbols');

// Each constant tracks how many callbacks there are for any given step of
// async execution. These are tracked so if the user didn't include callbacks
Expand Down Expand Up @@ -178,11 +178,13 @@ function fatalError(e) {

function lookupPublicResource(resource) {
if (typeof resource !== 'object' || resource === null) return resource;
// TODO(addaleax): Merge this with owner_symbol and use it across all
// AsyncWrap instances.
RaisinTen marked this conversation as resolved.
Show resolved Hide resolved
const publicResource = resource[resource_symbol];
if (publicResource !== undefined)

const publicResource = resource[owner_symbol];

if (publicResource != null) {
return publicResource;
}

return resource;
}

Expand Down
50 changes: 45 additions & 5 deletions lib/internal/js_stream_socket.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,55 @@ const kCurrentWriteRequest = Symbol('kCurrentWriteRequest');
const kCurrentShutdownRequest = Symbol('kCurrentShutdownRequest');
const kPendingShutdownRequest = Symbol('kPendingShutdownRequest');

function isClosing() { return this[owner_symbol].isClosing(); }
function isClosing() {
let socket = this[owner_symbol];

function onreadstart() { return this[owner_symbol].readStart(); }
if (socket.constructor.name === 'ReusedHandle') {
socket = socket.handle;
}

return socket.isClosing();
}

function onreadstart() {
let socket = this[owner_symbol];

if (socket.constructor.name === 'ReusedHandle') {
socket = socket.handle;
}

return socket.readStart();
}

function onreadstop() {
let socket = this[owner_symbol];

if (socket.constructor.name === 'ReusedHandle') {
socket = socket.handle;
}

return socket.readStop();
}

function onshutdown(req) {
let socket = this[owner_symbol];

if (socket.constructor.name === 'ReusedHandle') {
socket = socket.handle;
}

function onreadstop() { return this[owner_symbol].readStop(); }
return socket.doShutdown(req);
}

function onshutdown(req) { return this[owner_symbol].doShutdown(req); }
function onwrite(req, bufs) {
let socket = this[owner_symbol];

function onwrite(req, bufs) { return this[owner_symbol].doWrite(req, bufs); }
if (socket.constructor.name === 'ReusedHandle') {
socket = socket.handle;
}

return socket.doWrite(req, bufs);
}

/* This class serves as a wrapper for when the C++ side of Node wants access
* to a standard JS stream. For example, TLS or HTTP do not operate on network
Expand Down
13 changes: 11 additions & 2 deletions lib/internal/stream_base_commons.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ function handleWriteReq(req, data, encoding) {
function onWriteComplete(status) {
debug('onWriteComplete', status, this.error);

const stream = this.handle[owner_symbol];
let stream = this.handle[owner_symbol];

if (stream.constructor.name === 'ReusedHandle') {
stream = stream.handle;
}

if (stream.destroyed) {
if (typeof this.callback === 'function')
Expand Down Expand Up @@ -168,7 +172,12 @@ function onStreamRead(arrayBuffer) {
const nread = streamBaseState[kReadBytesOrError];

const handle = this;
const stream = this[owner_symbol];

let stream = this[owner_symbol];

if (stream.constructor.name === 'ReusedHandle') {
stream = stream.handle;
}

stream[kUpdateTimer]();

Expand Down
6 changes: 5 additions & 1 deletion lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -1102,7 +1102,11 @@ Socket.prototype.unref = function() {


function afterConnect(status, handle, req, readable, writable) {
const self = handle[owner_symbol];
let self = handle[owner_symbol];

if (self.constructor.name === 'ReusedHandle') {
self = self.handle;
}

// Callback may come after call to destroy
if (self.destroyed) {
Expand Down
4 changes: 2 additions & 2 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ void AsyncWrap::EmitDestroy(bool from_gc) {

if (!persistent().IsEmpty() && !from_gc) {
HandleScope handle_scope(env()->isolate());
USE(object()->Set(env()->context(), env()->resource_symbol(), object()));
USE(object()->Set(env()->context(), env()->owner_symbol(), object()));
}
RaisinTen marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down Expand Up @@ -586,7 +586,7 @@ void AsyncWrap::AsyncReset(Local<Object> resource, double execution_async_id,
Local<Object> obj = object();
CHECK(!obj.IsEmpty());
if (resource != obj) {
USE(obj->Set(env()->context(), env()->resource_symbol(), resource));
USE(obj->Set(env()->context(), env()->owner_symbol(), resource));
}
RaisinTen marked this conversation as resolved.
Show resolved Hide resolved
}
RaisinTen marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
1 change: 0 additions & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ constexpr size_t kFsStatsBufferLength =
V(oninit_symbol, "oninit") \
V(owner_symbol, "owner_symbol") \
V(onpskexchange_symbol, "onpskexchange") \
V(resource_symbol, "resource_symbol") \
V(trigger_async_id_symbol, "trigger_async_id_symbol") \

// Strings are per-isolate primitives but Environment proxies them
Expand Down
2 changes: 0 additions & 2 deletions test/async-hooks/test-http-agent-handle-reuse-parallel.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,4 @@ function onExit() {
// Verify reuse handle has been wrapped
assert.strictEqual(first.type, second.type);
assert.ok(first.handle !== second.handle, 'Resource reused');
assert.ok(first.handle === second.handle.handle,
'Resource not wrapped correctly');
}
2 changes: 0 additions & 2 deletions test/async-hooks/test-http-agent-handle-reuse-serial.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,4 @@ function onExit() {
// Verify reuse handle has been wrapped
assert.strictEqual(first.type, second.type);
assert.ok(first.handle !== second.handle, 'Resource reused');
assert.ok(first.handle === second.handle.handle,
'Resource not wrapped correctly');
}