Skip to content

Commit

Permalink
process: remove _getActive* functions in favour of async_hooks
Browse files Browse the repository at this point in the history
I don't think we need the process._getActiveRequests() and
process._getActiveHandles() functions anymore given that the async_hooks
module already helps us to keep a track of the active resources/handles.

Signed-off-by: Darshan Sen <[email protected]>
  • Loading branch information
RaisinTen committed Nov 13, 2021
1 parent dd52c05 commit 82fe642
Show file tree
Hide file tree
Showing 7 changed files with 69 additions and 63 deletions.
4 changes: 0 additions & 4 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,6 @@ const rawMethods = internalBinding('process_methods');
process.dlopen = rawMethods.dlopen;
process.uptime = rawMethods.uptime;

// TODO(joyeecheung): either remove them or make them public
process._getActiveRequests = rawMethods._getActiveRequests;
process._getActiveHandles = rawMethods._getActiveHandles;

// TODO(joyeecheung): remove these
process.reallyExit = rawMethods.reallyExit;
process._kill = rawMethods._kill;
Expand Down
35 changes: 0 additions & 35 deletions src/node_process_methods.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ typedef int mode_t;

namespace node {

using v8::Array;
using v8::ArrayBuffer;
using v8::BackingStore;
using v8::CFunction;
Expand Down Expand Up @@ -249,36 +248,6 @@ static void Uptime(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(result);
}

static void GetActiveRequests(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

std::vector<Local<Value>> request_v;
for (ReqWrapBase* req_wrap : *env->req_wrap_queue()) {
AsyncWrap* w = req_wrap->GetAsyncWrap();
if (w->persistent().IsEmpty())
continue;
request_v.emplace_back(w->GetOwner());
}

args.GetReturnValue().Set(
Array::New(env->isolate(), request_v.data(), request_v.size()));
}

// Non-static, friend of HandleWrap. Could have been a HandleWrap method but
// implemented here for consistency with GetActiveRequests().
void GetActiveHandles(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

std::vector<Local<Value>> handle_v;
for (auto w : *env->handle_wrap_queue()) {
if (!HandleWrap::HasRef(w))
continue;
handle_v.emplace_back(w->GetOwner());
}
args.GetReturnValue().Set(
Array::New(env->isolate(), handle_v.data(), handle_v.size()));
}

static void ResourceUsage(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down Expand Up @@ -558,8 +527,6 @@ static void InitializeProcessMethods(Local<Object> target,
env->SetMethod(target, "cpuUsage", CPUUsage);
env->SetMethod(target, "resourceUsage", ResourceUsage);

env->SetMethod(target, "_getActiveRequests", GetActiveRequests);
env->SetMethod(target, "_getActiveHandles", GetActiveHandles);
env->SetMethod(target, "_kill", Kill);

env->SetMethodNoSideEffect(target, "cwd", Cwd);
Expand All @@ -585,8 +552,6 @@ void RegisterProcessMethodsExternalReferences(
registry->Register(CPUUsage);
registry->Register(ResourceUsage);

registry->Register(GetActiveRequests);
registry->Register(GetActiveHandles);
registry->Register(Kill);

registry->Register(Cwd);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
'use strict';

require('../common');

const assert = require('assert');
const async_hooks = require('async_hooks');
const net = require('net');

const activeHandles = new Map();
async_hooks.createHook({
init(asyncId, type, triggerAsyncId, resource) {
if (['TCPSERVERWRAP', 'TCPWRAP'].includes(type))
activeHandles.set(asyncId, resource);
},
destroy(asyncId) {
activeHandles.delete(asyncId);
}
}).enable();

const NUM = 8;
const connections = [];
const clients = [];
Expand Down Expand Up @@ -30,18 +44,18 @@ function clientConnected(client) {


function checkAll() {
const handles = process._getActiveHandles();
const handles = Array.from(activeHandles.values());

clients.forEach(function(item) {
assert.ok(handles.includes(item));
assert.ok(handles.includes(item._handle));
item.destroy();
});

connections.forEach(function(item) {
assert.ok(handles.includes(item));
assert.ok(handles.includes(item._handle));
item.end();
});

assert.ok(handles.includes(server));
assert.ok(handles.includes(server._handle));
server.close();
}
23 changes: 23 additions & 0 deletions test/parallel/test-async-hooks-track-active-requests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

const common = require('../common');

const assert = require('assert');
const async_hooks = require('async_hooks');
const fs = require('fs');

let numFsReqCallbacks = 0;
async_hooks.createHook({
init(asyncId, type, triggerAsyncId, resource) {
if (type === 'FSREQCALLBACK')
++numFsReqCallbacks;
},
destroy(asyncId) {
--numFsReqCallbacks;
}
}).enable();

for (let i = 0; i < 12; i++)
fs.open(__filename, 'r', common.mustCall());

assert.strictEqual(numFsReqCallbacks, 12);
10 changes: 0 additions & 10 deletions test/parallel/test-process-getactiverequests.js

This file was deleted.

10 changes: 3 additions & 7 deletions test/pseudo-tty/ref_keeps_node_running.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,14 @@ const handle = new TTY(0);
handle.readStart();
handle.onread = () => {};

function isHandleActive(handle) {
return process._getActiveHandles().some((active) => active === handle);
}

strictEqual(isHandleActive(handle), true, 'TTY handle not initially active');
strictEqual(handle.hasRef(), true, 'TTY handle not initially active');

handle.unref();

strictEqual(isHandleActive(handle), false, 'TTY handle active after unref()');
strictEqual(handle.hasRef(), false, 'TTY handle active after unref()');

handle.ref();

strictEqual(isHandleActive(handle), true, 'TTY handle inactive after ref()');
strictEqual(handle.hasRef(), true, 'TTY handle inactive after ref()');

handle.unref();
28 changes: 25 additions & 3 deletions test/sequential/test-net-connect-econnrefused.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,30 @@
// Verify that connect reqs are properly cleaned up.

const common = require('../common');

const assert = require('assert');
const async_hooks = require('async_hooks');
const net = require('net');

const activeRequestsMap = new Map();
const activeHandlesMap = new Map();
async_hooks.createHook({
init(asyncId, type, triggerAsyncId, resource) {
switch (type) {
case 'TCPCONNECTWRAP':
activeRequestsMap.set(asyncId, resource);
break;
case 'TCPWRAP':
activeHandlesMap.set(asyncId, resource);
break;
}
},
destroy(asyncId) {
activeRequestsMap.delete(asyncId);
activeHandlesMap.delete(asyncId);
}
}).enable();

const ROUNDS = 5;
const ATTEMPTS_PER_ROUND = 50;
let rounds = 1;
Expand Down Expand Up @@ -54,9 +75,10 @@ function pummel() {

function check() {
setTimeout(common.mustCall(function() {
assert.strictEqual(process._getActiveRequests().length, 0);
const activeHandles = process._getActiveHandles();
assert.ok(activeHandles.every((val) => val.constructor.name !== 'Socket'));
const activeRequests = Array.from(activeRequestsMap.values());
assert.strictEqual(activeRequests.length, 0);
const activeHandles = Array.from(activeHandlesMap.values());
assert.ok(activeHandles.every((val) => val.constructor.name === 'TCP'));
}), 0);
}

Expand Down

0 comments on commit 82fe642

Please sign in to comment.