Skip to content

Conversation

@alexkozy
Copy link
Member

@alexkozy alexkozy commented Aug 18, 2018

This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this flag for
every execution in console.

Fixes: #22157

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

@alexkozy alexkozy requested a review from addaleax August 18, 2018 02:00
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 18, 2018
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t the close callback free the memory allocated for handle here? Or, alternatively …

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we should, fixed.

Copy link
Member

Choose a reason for hiding this comment

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

… I think we could use the fact that std::set comes with retaining referential integrity in the presence of insert/erase operations, so we could avoid the double allocations and use std::set<uv_timer_t> instead, right? (If not: I don’t think there’s an advantage over std::unordered_set, right?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Explicit allocation looks simpler to me, so I replaced set with unordered_set. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

Nit: scheduler = ContainerOf(&DelayedTaskScheduler::loop_, timer->loop);

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@addaleax
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2018
@joyeecheung
Copy link
Member

This PR needs a rebase against master to avoid the git failure in the CI.

@alexkozy
Copy link
Member Author

I rebased and restarted CI: https://ci.nodejs.org/job/node-test-pull-request/16589/

This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Fixes: #22157
@alexkozy
Copy link
Member Author

Landed in b1e2612

@alexkozy alexkozy closed this Aug 21, 2018
alexkozy added a commit that referenced this pull request Aug 21, 2018
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

PR-URL: #22383
Fixes: #22157
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@alexkozy alexkozy deleted the implemented-post-delayed-task branch August 21, 2018 01:11
@addaleax
Copy link
Member

@ak239 Can you open a backport PR against v10.x-staging here (→ guide), since this doesn’t apply cleanly, unfortunately?

@alexkozy
Copy link
Member Author

I uploaded rebased PR and started CI there, I won't have internet connection next 12 hours, so I would appreciate help with merging my backport, thanks ahead!

addaleax pushed a commit that referenced this pull request Aug 28, 2018
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Backport-PR-URL: #22567
PR-URL: #22383
Fixes: #22157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 28, 2018
targos pushed a commit that referenced this pull request Sep 3, 2018
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Backport-PR-URL: #22567
PR-URL: #22383
Fixes: #22157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2018
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Backport-PR-URL: #22567
PR-URL: #22383
Fixes: #22157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
blattersturm pushed a commit to citizenfx/node that referenced this pull request Nov 3, 2018
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

PR-URL: nodejs#22383
Fixes: nodejs#22157
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. v8 platform Issues and PRs related to Node's v8::Platform implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UNREACHABLE in WorkerThreadsTaskRunner::PostDelayedTask

7 participants