Validate maximum thread count - #2090
Conversation
tmccombs
left a comment
tmccombs
left a comment
There was a problem hiding this comment.
While values below 64 are probably safe, I think that is probably too low for a hard-coded limit.
A fundamental problem here is that the maximum number of threads that won't result in an error is basically impossible to know.
It depends on a variety of factors, including OS, amount of available memory, ulimit settings for the current process, etc. I think something like say 2^16 is probably reasonable to use as a limit, but 64 is definitely too low. That means that on, say, a 128 core computer, you can't even use all of your cores.
| use crate::filter::OwnerFilter; | ||
| use crate::filter::SizeFilter; | ||
|
|
||
| const MAX_NUM_THREADS: usize = 64; |
There was a problem hiding this comment.
Why 64?
The number of threads can be significantly higher than that. Especially on hosts with a large number of cpu cores.
| /// Set number of threads to use for searching & executing (default: number | ||
| /// of available CPU cores) | ||
| #[arg(long, short = 'j', value_name = "num", hide_short_help = true, value_parser = str::parse::<NonZeroUsize>)] | ||
| #[arg(long, short = 'j', value_name = "num", hide_short_help = true, value_parser = parse_num_threads)] |
There was a problem hiding this comment.
why not use clap's built-in support for ranges?
2 participants
Summary
Validate
--threadsduring CLI argument parsing and reject values greater than the supported maximum (64).Previously, excessively large values could be accepted and later trigger panics or aborts during channel allocation or thread creation. This change surfaces those cases as a normal CLI validation error instead.
Changes
parse_num_threadsparser for--threadsMAX_NUM_THREADS(64)MAX_NUM_THREADSfor the default thread limit--threadsvaluesFixes
Closes #2078