MOD-13142 Option to resize mr thpool after creation, but not after started#89
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| return REDISMODULE_ERR; | ||
| } | ||
|
|
||
| if (mr_thpool_resize_unstarted(tp, numThreads) != 0) { |
There was a problem hiding this comment.
Implicit size_t-to-int narrowing truncates thread count
Medium Severity
MR_ResizeExecutionThreadPoolIfUnstarted accepts size_t numThreads but passes it directly to mr_thpool_resize_unstarted, which takes int num_threads. On 64-bit systems, values exceeding INT_MAX are silently truncated. The <= 0 guard inside the callee catches some overflow cases but not all — a size_t like (1UL << 32) + 5 truncates to 5, passing validation and allocating the wrong number of thread slots.
Additional Locations (1)
galcohen-redislabs
left a comment
galcohen-redislabs
left a comment
There was a problem hiding this comment.
Also: what happened to Tom's tests?
|
|
||
| if (num_threads < 0) { | ||
| num_threads = 0; | ||
| if (num_threads <= 0) { |
There was a problem hiding this comment.
Why the old code allowed 0? Was it "unlimited" maybe?
There was a problem hiding this comment.
I dont think so. When it was 0, its just kept allocating 0 bytes for the threads, which is 0 threads(and is there a reason we should allow it)?
Will be in TS repo on the next PR |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| num_threads = 0; | ||
| if (num_threads <= 0) { | ||
| err("thpool_init(): num_threads must be greater than 0\n"); | ||
| return NULL; |
There was a problem hiding this comment.
mr_thpool_init returning NULL for zero is unchecked
Medium Severity
mr_thpool_init now returns NULL when num_threads is 0, but its only caller MR_Init does not check the return value and unconditionally returns REDISMODULE_OK. Previously, passing 0 created a valid (though empty) pool object. Now, mrCtx.executionsThreadPool is left NULL, and the first call to mr_thpool_add_work will dereference it in mr_thpool_start_threads, causing a NULL pointer crash.
Additional Locations (1)
2 participants
Note
Medium Risk
Touches core threading infrastructure:
mr_thpool_initnow rejects non-positive sizes and new resize logic reallocates the thread table, which could affect startup/config flows and error handling if callers previously passed 0 or resize at the wrong time.Overview
Adds a public API (
MR_ResizeExecutionThreadPoolIfUnstarted) to change the execution thread-pool size afterMR_Init()creates the pool but before any worker threads are started, returning an error once work has begun.Extends the threadpool implementation with
mr_thpool_workers_started()andmr_thpool_resize_unstarted()to support this guard/resize behavior, and tightensmr_thpool_init()to fail (rather than silently creating a 0-thread pool) whennum_threads <= 0.Written by Cursor Bugbot for commit 1e32f98. This will update automatically on new commits. Configure here.