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.

Sign up for GitHub

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

Jump to bottom

async_hooks: run destroy callbacks before normal exit #13286

Closed
wants to merge 1 commit into from
Closed

async_hooks: run destroy callbacks before normal exit #13286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions src/async-wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,13 @@ static void DestroyIdsCb(uv_idle_t* handle) {
uv_idle_stop(handle);

Environment* env = Environment::from_destroy_ids_idle_handle(handle);

HandleScope handle_scope(env->isolate());
Context::Scope context_scope(env->context());

AsyncWrap::RunDestroyCbs(env);
}

void AsyncWrap::RunDestroyCbs(Environment* env) {
Local<Function> fn = env->async_hooks_destroy_function();

TryCatch try_catch(env->isolate());
Expand All @@ -164,8 +168,6 @@ static void DestroyIdsCb(uv_idle_t* handle) {
FatalException(env->isolate(), try_catch);
}
}

env->destroy_ids_list()->clear();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this fixes the issue tested by parallel/test-async-hooks-close-during-destroy. If somebody feels like I should, I can split it into a second commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be nice for the purpose of adding a good commit message descibing the issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax If you don't clear the std::vector<double> then destroy ids list will continue to call the destroy hooks for all the previously entered asyncIds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris Just before the loop above, you already swapped the list with an empty vector, which is the right way to solve that. Re-clearing the list may actually clear too much (you can try out the test file here yourself, if you like).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just before the loop above, you already swapped the list with an empty vector, which is the right way to solve that.

ah yup. you're right.

}


Expand Down
2 changes: 2 additions & 0 deletions src/async-wrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ class AsyncWrap : public BaseObject {
static bool EmitBefore(Environment* env, double id);
static bool EmitAfter(Environment* env, double id);

static void RunDestroyCbs(Environment* env);

inline ProviderType provider_type() const;

inline double get_id() const;
Expand Down
2 changes: 2 additions & 0 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4531,6 +4531,8 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
} while (more == true);
}

AsyncWrap::RunDestroyCbs(&env);
Copy link
Member

@AndreasMadsen AndreasMadsen May 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If destroy creates a new asynchronous resource (think logging) then that won't be executed. This is why I think we should treat the destroy queue exactly like the setImmediate queue, by using uv_check_start.

I also don't like to pollute the event loop code if it can be handled by libuv.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately that seems to change the timing of the destroy callback visibly and makes a couple of tests fail. I’ll try to get to it tomorrow.

Copy link
Contributor

@trevnorris trevnorris May 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I think we should treat the destroy queue exactly like the setImmediate queue, by using uv_check_start.

You mean you'd prefer we use uv_check_t instead of a uv_idle_t as it's done in PushBackDestroyId() (src/async-wrap.cc)?

Honestly if you want the callbacks to run before the poll phase then it might be worth it to run it as a uv_timer_t with a timeout of 0. This will cause them to be run before any setImmediate()'s are called.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax ah. think I see what's going on. will get another fix for this in a moment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris Thanks – I’ll pick the env->destroy_ids_list()->clear(); changes above into another PR and let you take it from here. I’ll close this once your PR is up. 👍


env.set_trace_sync_io(false);

const int exit_code = EmitExit(&env);
Expand Down
38 changes: 38 additions & 0 deletions test/parallel/test-async-hooks-close-during-destroy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';
// Test that async ids that are added to the destroy queue while running a
// `destroy` callback are handled correctly.

const common = require('../common');
const assert = require('assert');
const net = require('net');
const async_hooks = require('async_hooks');

const initCalls = new Set();
let destroyTcpWrapCallCount = 0;
let srv2;

async_hooks.createHook({
init: common.mustCallAtLeast((id, provider, triggerId) => {
if (provider === 'TCPWRAP')
initCalls.add(id);
}, 2),
destroy: common.mustCallAtLeast((id) => {
if (!initCalls.has(id)) return;

switch (destroyTcpWrapCallCount++) {
case 0:
// Trigger the second `destroy` call.
srv2.close();
break;
case 2:
assert.fail('More than 2 destroy() invocations');
break;
}
}, 2)
}).enable();

// Create a server to trigger the first `destroy` callback.
net.createServer().listen(0).close();
srv2 = net.createServer().listen(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This abuses node internals. We should assume that bind() is always async, as if a hostname needed to be resolved. e.g. the following causes the server to never close:

net.createServer().listen({port: 0, host: 'localhost'}).close();

Instead it needs to be written as such:

net.createServer().listen({port: 0, host: 'localhost'}, function() { this.close() });

This means you can't reliably call srv2.close() from the destroy callback the way is being done here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IOW, the those two lines should instead be written as such:

srv2 = net.createServer().listen(0, () => {
  net.createServer().listen(0, function() { this.close() });
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris Is there anything in Node that always closes synchronously? Anyway, I’ve updated #13353 to just use AsyncResource instead, and I’d suggest we continue the review there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax everything libuv that "closes" always goes through uv_close() and so will be called async.


process.on('exit', () => assert.strictEqual(destroyTcpWrapCallCount, 2));
27 changes: 27 additions & 0 deletions test/parallel/test-async-hooks-top-level-clearimmediate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';

// Regression test for https://github.com/nodejs/node/issues/13262

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

let seenId, seenResource;

async_hooks.createHook({
init: common.mustCall((id, provider, triggerId, resource) => {
seenId = id;
seenResource = resource;
assert.strictEqual(provider, 'Immediate');
assert.strictEqual(triggerId, 1);
}),
before: common.mustNotCall(),
after: common.mustNotCall(),
destroy: common.mustCall((id) => {
assert.strictEqual(seenId, id);
})
}).enable();

const immediate = setImmediate(common.mustNotCall());
assert.strictEqual(immediate, seenResource);
clearImmediate(immediate);