Allow dynamic modification of io-threads num by ayush933 · Pull Request #2033 · valkey-io/valkey

ayush933 · 2025-04-30T15:36:55Z

Item from #761

This PR has the following changes

Bug fix where calling pthread_join() from main thread for an IO thread would hang indefinitely. This is because IOThreadMain() doesn't have a cancellation point.So pthread_cancel() from main thread is not honored.
Can be reproed by calling shutdownIOThread() from the main thread for any active thread with empty job queue.
Fixed by adding cancellation point in IOThreadMain().
Makes io-threads config runtime modifiable.

ayush933 · 2025-04-30T15:42:50Z

UTs pending atm.

codecov · 2025-04-30T15:52:57Z

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 71.37%. Comparing base (af25a52) to head (3f54996).
⚠️ Report is 639 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/io_threads.c	96.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2033      +/-   ##
============================================
+ Coverage     71.02%   71.37%   +0.34%     
============================================
  Files           122      122              
  Lines         66171    66050     -121     
============================================
+ Hits          46999    47142     +143     
+ Misses        19172    18908     -264

Files with missing lines	Coverage Δ
src/config.c	`78.52% <ø> (+0.12%)`	⬆️
src/networking.c	`87.44% <100.00%> (+0.26%)`	⬆️
src/server.h	`100.00% <ø> (ø)`
src/io_threads.c	`34.70% <96.00%> (+27.89%)`	⬆️

... and 17 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.

madolson

Nice. Just minor things, looking forward to the tests.

ayush933 · 2025-05-01T08:45:03Z

Hey @uriyage, just curious , can you please explain the safety concerns a bit more i.e. why we would want to drain all jobs and inactivate all threads instead of the min num required?

uriyage · 2025-05-01T15:26:41Z

Hey @uriyage, just curious , can you please explain the safety concerns a bit more i.e. why we would want to drain all jobs and inactivate all threads instead of the min num required?

@ayush933 I don't recall the exact workflow, but I remember attempting something similar, and it affected the logic of dynamically adjusting the threads or the client cur_tid logic. I might be mistaken, but I think it will be cleaner and safer.

ayush933 · 2025-05-08T09:38:06Z

Hey @uriyage @madolson , anything else needed to move this PR forward?

madolson

Just some nits, but still would like @uriyage to take sign off as well, he knows more about this code.

uriyage · 2025-05-11T11:06:57Z

Just some nits, but still would like @uriyage to take sign off as well, he knows more about this code.

LGTM. I left one small comment about the log verbosity

Signed-off-by: Ayush Sharma <mrayushs933@gmail.com>

zuiderkwast

Conceptually approve. I didn't review the code (just looked briefly).

Bug fix where calling pthread_join() from main thread for an IO thread would hang indefinitely

What is the severity of this bug? Which scenario triggers it, from a user's perspective? Do we need to backport the bugfix? (If we need that, may need to split out the bugfix to a separate commit that we can backport, since this PRs is a new feature + a bugfix. We should not backport the new feature.)

ayush933 · 2025-05-17T05:33:17Z

Before this feature, the only time we called shutdownIoThread() was on server shutdown.
Considering this wasn't noticed until now, I don't think its too severe and needs a backport but lets wait for @uriyage's input on this.

uriyage · 2025-05-19T09:14:12Z

Before this feature, the only time we called shutdownIoThread() was on server shutdown. Considering this wasn't noticed until now, I don't think its too severe and needs a backport but lets wait for @uriyage's input on this.

Currently, it is only used by doFastMemoryTest, which is called from printCrashReport after a crash occurs, in this case the threads are suspended by the ThreadsManager_runOnThreads so not an issue.

madolson · 2025-05-28T23:08:05Z

New crash https://github.com/valkey-io/valkey/actions/runs/15312340723/job/43079580795?pr=2149#step:4:1444

Item from valkey-io#761 This PR has the following changes 1. Bug fix where calling `pthread_join()` from main thread for an IO thread would hang indefinitely. This is because `IOThreadMain()` doesn't have a cancellation point.So `pthread_cancel()` from main thread is not honored. Can be reproed by calling `shutdownIOThread()` from the main thread for any active thread with empty job queue. Fixed by adding cancellation point in `IOThreadMain()`. 2. Makes `io-threads` config runtime modifiable. Signed-off-by: Ayush Sharma <mrayushs933@gmail.com> Signed-off-by: chzhoo <czawyx@163.com>

Item from valkey-io#761 This PR has the following changes 1. Bug fix where calling `pthread_join()` from main thread for an IO thread would hang indefinitely. This is because `IOThreadMain()` doesn't have a cancellation point.So `pthread_cancel()` from main thread is not honored. Can be reproed by calling `shutdownIOThread()` from the main thread for any active thread with empty job queue. Fixed by adding cancellation point in `IOThreadMain()`. 2. Makes `io-threads` config runtime modifiable. Signed-off-by: Ayush Sharma <mrayushs933@gmail.com> Signed-off-by: shanwan1 <shanwan1@intel.com>

ayush933 marked this pull request as ready for review April 30, 2025 20:15

madolson reviewed Apr 30, 2025

View reviewed changes

Comment thread src/io_threads.c Outdated

Comment thread src/io_threads.c Outdated

uriyage reviewed May 1, 2025

View reviewed changes

Comment thread src/io_threads.c Outdated

Comment thread src/io_threads.c Outdated

Comment thread src/config.c

ayush933 force-pushed the dynamicThreads branch 2 times, most recently from 451df91 to cde1b5a Compare May 1, 2025 15:24

ayush933 force-pushed the dynamicThreads branch 2 times, most recently from 40b1d3f to c450698 Compare May 1, 2025 19:10

ayush933 requested review from madolson and uriyage May 1, 2025 19:13

ayush933 force-pushed the dynamicThreads branch from c450698 to 443bba2 Compare May 4, 2025 05:43

uriyage reviewed May 4, 2025

View reviewed changes

Comment thread tests/unit/other.tcl

ayush933 force-pushed the dynamicThreads branch from 443bba2 to dea8c30 Compare May 4, 2025 12:27

ayush933 requested a review from uriyage May 5, 2025 14:33

ayush933 force-pushed the dynamicThreads branch from dea8c30 to 6e2600d Compare May 6, 2025 07:57

madolson approved these changes May 8, 2025

View reviewed changes

Comment thread src/server.h Outdated

Comment thread tests/unit/other.tcl Outdated

Comment thread tests/unit/other.tcl Outdated

Comment thread tests/unit/other.tcl Outdated

ayush933 force-pushed the dynamicThreads branch from 6e2600d to 893030b Compare May 9, 2025 03:31

uriyage approved these changes May 11, 2025

View reviewed changes

Comment thread src/io_threads.c Outdated

ayush933 force-pushed the dynamicThreads branch 2 times, most recently from 500689b to 5698821 Compare May 11, 2025 15:16

madolson reviewed May 12, 2025

View reviewed changes

Comment thread tests/unit/other.tcl

ayush933 force-pushed the dynamicThreads branch from 5698821 to 7eb0084 Compare May 15, 2025 14:42

zuiderkwast reviewed May 16, 2025

View reviewed changes

Comment thread src/server.h Outdated

zuiderkwast added release-notes This issue should get a line item in the release notes to-be-merged Almost ready to merge labels May 16, 2025

Allow dynamic modification of io-threads num

3f54996

Signed-off-by: Ayush Sharma <mrayushs933@gmail.com>

ayush933 force-pushed the dynamicThreads branch from 7eb0084 to 3f54996 Compare May 16, 2025 14:28

zuiderkwast approved these changes May 16, 2025

View reviewed changes

zhulipeng reviewed May 23, 2025

View reviewed changes

Comment thread src/config.c

madolson merged commit 4aaaf88 into valkey-io:unstable May 28, 2025

uriyage reviewed May 29, 2025

View reviewed changes

Comment thread src/io_threads.c

enjoy-binbin mentioned this pull request Apr 15, 2026

Followup items from #758 Async IO threads #761

Open

10 tasks

Uh oh!

Conversation

ayush933 commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ayush933 commented Apr 30, 2025

Uh oh!

codecov Bot commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ayush933 commented May 1, 2025

Uh oh!

uriyage commented May 1, 2025

Uh oh!

Uh oh!

ayush933 commented May 8, 2025

Uh oh!

madolson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uriyage commented May 11, 2025

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

ayush933 commented May 17, 2025

Uh oh!

uriyage commented May 19, 2025

Uh oh!

Uh oh!

madolson commented May 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ayush933 commented Apr 30, 2025 •

edited

Loading

codecov Bot commented Apr 30, 2025 •

edited

Loading

madolson left a comment •

edited

Loading