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_wrap: clear destroy_ids vector #10400

Merged
merged 1 commit into from Dec 22, 2016

Conversation

Projects
None yet
5 participants
@trevnorris
Contributor

trevnorris commented Dec 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

async_wrap

Description of change

After processing all the callbacks in the destroy_ids vector make sure
to clear() it otherwise the DestroyIdsCb() won't run again.

This patch should land on all releases that have b49b496, or some cherry-pick of it.

CI: https://ci.nodejs.org/job/node-test-commit/6782/

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Dec 22, 2016

Contributor

All failing AIX tests not related to this change.

Contributor

trevnorris commented Dec 22, 2016

All failing AIX tests not related to this change.

@bnoordhuis

Left a suggestion but LGTM either way.

@@ -212,6 +212,8 @@ void AsyncWrap::DestroyIdsCb(uv_idle_t* handle) {
FatalException(env->isolate(), try_catch);
}
}
env->destroy_ids_list()->clear();

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 22, 2016

Member

Can I suggest you swap before iteration? I.e.:

std::vector<int64_t> destroy_ids_list;
destroy_ids_list.swap(*env->destroy_ids_list());
for (auto current_id : destroy_ids_list) {
  // ...
}

That way the list is both cleared and immune to concurrent modification by the callback.

@bnoordhuis

bnoordhuis Dec 22, 2016

Member

Can I suggest you swap before iteration? I.e.:

std::vector<int64_t> destroy_ids_list;
destroy_ids_list.swap(*env->destroy_ids_list());
for (auto current_id : destroy_ids_list) {
  // ...
}

That way the list is both cleared and immune to concurrent modification by the callback.

This comment has been minimized.

@trevnorris

trevnorris Dec 22, 2016

Contributor

done

@trevnorris

trevnorris Dec 22, 2016

Contributor

done

@addaleax

LGTM with Ben’s suggestion

async_wrap: clear destroy_ids vector
After processing all the callbacks in the destroy_ids vector make sure
to clear() it otherwise the DestroyIdsCb() won't run again.

PR-URL: #10400
Fixes: b49b496 "async_wrap: call destroy() callback in uv_idle_t"
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@trevnorris trevnorris merged commit 833294f into nodejs:master Dec 22, 2016

trevnorris added a commit that referenced this pull request Dec 22, 2016

async_wrap: clear destroy_ids vector
After processing all the callbacks in the destroy_ids vector make sure
to clear() it otherwise the DestroyIdsCb() won't run again.

PR-URL: #10400
Fixes: b49b496 "async_wrap: call destroy() callback in uv_idle_t"
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Dec 22, 2016

Contributor

Thank you. Merged, and landed on v7.x-staging in 81a6dd5.

@thealphanerd This should land in v6.x before the next release, if possible. I'll wait until this lands on v6, then prep both this and #9174 for v4.

Contributor

trevnorris commented Dec 22, 2016

Thank you. Merged, and landed on v7.x-staging in 81a6dd5.

@thealphanerd This should land in v6.x before the next release, if possible. I'll wait until this lands on v6, then prep both this and #9174 for v4.

@evanlucas evanlucas referenced this pull request Jan 3, 2017

Merged

v7.4.0 release proposal #10589

evanlucas added a commit that referenced this pull request Jan 3, 2017

async_wrap: clear destroy_ids vector
After processing all the callbacks in the destroy_ids vector make sure
to clear() it otherwise the DestroyIdsCb() won't run again.

PR-URL: #10400
Fixes: b49b496 "async_wrap: call destroy() callback in uv_idle_t"
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Jan 23, 2017

async_wrap: clear destroy_ids vector
After processing all the callbacks in the destroy_ids vector make sure
to clear() it otherwise the DestroyIdsCb() won't run again.

PR-URL: #10400
Fixes: b49b496 "async_wrap: call destroy() callback in uv_idle_t"
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jan 23, 2017

Member

@trevnorris this has been backported to v6.x and will be in the next release

Member

MylesBorins commented Jan 23, 2017

@trevnorris this has been backported to v6.x and will be in the next release

MylesBorins added a commit that referenced this pull request Jan 24, 2017

async_wrap: clear destroy_ids vector
After processing all the callbacks in the destroy_ids vector make sure
to clear() it otherwise the DestroyIdsCb() won't run again.

PR-URL: #10400
Fixes: b49b496 "async_wrap: call destroy() callback in uv_idle_t"
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@MylesBorins MylesBorins referenced this pull request Jan 24, 2017

Merged

v6.10.0 proposal #10974

MylesBorins added a commit that referenced this pull request Jan 31, 2017

async_wrap: clear destroy_ids vector
After processing all the callbacks in the destroy_ids vector make sure
to clear() it otherwise the DestroyIdsCb() won't run again.

PR-URL: #10400
Fixes: b49b496 "async_wrap: call destroy() callback in uv_idle_t"
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment