Skip to content

src: improve thread safety of TaskQueue #57910

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Apr 17, 2025

Initial effort to fix #56236.

Improve thread safety of TaskQueue by making locking of TaskQueue explicit and thus hopefully preventing a crash due to uv_async_send being called with nullptr.

Please let me know if i headed in the wrong direction with this and i'll rework it!

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 17, 2025
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 97.80220% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.25%. Comparing base (5077ea4) to head (f1b6765).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
src/node_platform.cc 97.77% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #57910    +/-   ##
========================================
  Coverage   90.25%   90.25%            
========================================
  Files         630      630            
  Lines      185691   185944   +253     
  Branches    36407    36443    +36     
========================================
+ Hits       167589   167827   +238     
- Misses      10994    11012    +18     
+ Partials     7108     7105     -3     
Files with missing lines Coverage Δ
src/node_platform.h 80.00% <100.00%> (+5.00%) ⬆️
src/node_platform.cc 88.75% <97.77%> (+0.89%) ⬆️

... and 50 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina
Copy link
Member

@codebytere I wonder if this could also help with #54918.

@codebytere codebytere requested a review from bnoordhuis April 18, 2025 08:53
@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2025
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 18, 2025
@nodejs nodejs deleted a comment from github-actions bot Apr 18, 2025
@codebytere codebytere removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 18, 2025
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with some suggestions.

Comment on lines +107 to +114
std::vector<std::unique_ptr<Task>> tasks_to_run;
{
auto locked = scheduler->tasks_.Lock();
std::unique_ptr<Task> task;
while ((task = locked.Pop())) {
tasks_to_run.push_back(std::move(task));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::vector<std::unique_ptr<Task>> tasks_to_run;
{
auto locked = scheduler->tasks_.Lock();
std::unique_ptr<Task> task;
while ((task = locked.Pop())) {
tasks_to_run.push_back(std::move(task));
}
}
auto tasks_to_run = scheduler->tasks_.Lock().PopAll();

That's equivalent, right?

Comment on lines +482 to +492
std::vector<std::unique_ptr<DelayedTask>> delayed_tasks_to_schedule;
{
auto locked_tasks = foreground_delayed_tasks_.Lock();
std::unique_ptr<DelayedTask> delayed;
while ((delayed = locked_tasks.Pop())) {
did_work = true;
delayed_tasks_to_schedule.push_back(std::move(delayed));
}
}

for (auto& delayed : delayed_tasks_to_schedule) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above:

Suggested change
std::vector<std::unique_ptr<DelayedTask>> delayed_tasks_to_schedule;
{
auto locked_tasks = foreground_delayed_tasks_.Lock();
std::unique_ptr<DelayedTask> delayed;
while ((delayed = locked_tasks.Pop())) {
did_work = true;
delayed_tasks_to_schedule.push_back(std::move(delayed));
}
}
for (auto& delayed : delayed_tasks_to_schedule) {
auto delayed_tasks_to_schedule = foreground_delayed_tasks_.Lock().PopAll();
for (auto& delayed : delayed_tasks_to_schedule) {
did_work = true;

Comment on lines +313 to +314
auto foreground_tasks_locked = foreground_tasks_.Lock();
auto foreground_delayed_tasks_locked = foreground_delayed_tasks_.Lock();
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to their definitions in node_platform.h explaining the locks should always be acquired in this order?

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++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node.js 20 Upgrade: Segmentation Fault Core Dump During Pipeline Lage Build Step
4 participants