add max-os-threads flag
#5622
add max-os-threads flag
#5622
Conversation
|
We can do that, but I believe overriding it should be an option. |
dogancanbakir
force-pushed
the
add_most_flag
branch
from
ed31c49
to
94933db
Compare
There was a problem hiding this comment.
Since this is something valid for all the tools I think we should consider overidding the setting via environment variable and move this logic to the utils package that will evaluate within an init() function in any existing package providing similar functionalities, or eventually creating a new one named global, for example:
$ OS_THREADS=xxx nucleiWhat do you think?
There was a problem hiding this comment.
if possible i think we should skip adding this flag, don't exactly have a link to quote at the moment but i did read in golang repo or google form that if max threads threshold is being breached either something went wrong at go level (cgo or something else) or a usecase that might not be suitable for go , but almost all cases this setting would not be required to tweak ( even db's do not start 10k threads if i am not wrong )
also recently a contributor reported same issue when i asked if he was able to repro this on different machine it turned out the bug was in the machine / setup and not nuclei ( see: https://github.com/orgs/projectdiscovery/discussions/4767 )
|
@Mzack9999 makes sense. @tarunKoyalwar We focus only on maximizing limits. If we allow users to configure this, they will have the flexibility to increase the maximum limit and set it to a lower value if desired. |
WalkthroughThe pull request includes updates to the Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
depends on projectdiscovery/utils#572 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
pkg/types/types.go (3)
306-307: Enhance the field documentation with more details.The current comment should provide more context about the purpose, impact, and recommended values for this setting.
- // MaxOSThreads is the maximum number of OS threads to use + // MaxOSThreads is the maximum number of operating system threads to use. + // This setting helps prevent thread exhaustion when scanning with large numbers + // of templates. The default is 10000. Setting this too low may impact performance, + // while setting it too high may cause system resource issues.
Line range hint
590-603: Add default value for MaxOSThreads in DefaultOptions.The
MaxOSThreadsfield should have a sensible default value to prevent potential issues when not explicitly set.func DefaultOptions() *Options { return &Options{ RateLimit: 150, RateLimitDuration: time.Second, BulkSize: 25, TemplateThreads: 25, HeadlessBulkSize: 10, PayloadConcurrency: 25, HeadlessTemplateThreads: 10, ProbeConcurrency: 50, + MaxOSThreads: 10000, Timeout: 5, Retries: 1, MaxHostError: 30, ResponseReadSize: 10 * unitutils.Mega, ResponseSaveSize: unitutils.Mega, } }
306-307: Consider adding value validation for MaxOSThreads.To prevent potential system issues, it would be beneficial to add validation for the
MaxOSThreadsvalue to ensure it stays within reasonable bounds.Consider:
- Adding a minimum value check (e.g., >= 100) to ensure basic functionality
- Adding a maximum value check based on system capabilities
- Implementing this in a new validation method or extending existing validation logic
Example validation approach:
// ValidateThreads validates the MaxOSThreads configuration func (options *Options) ValidateThreads() error { if options.MaxOSThreads < 100 { return fmt.Errorf("max-os-threads must be >= 100") } // Consider system-specific maximum based on available resources if options.MaxOSThreads > 100000 { return fmt.Errorf("max-os-threads must be <= 100000") } return nil }cmd/nuclei/main.go (1)
405-405: LGTM: Well-structured flag definitionThe max-os-threads flag is appropriately placed in the optimization group with a clear description and sensible default value.
Consider adding documentation about:
- The implications of adjusting this value
- Recommended ranges based on system capabilities
- Potential impact on performance and stability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cmd/nuclei/main.go(3 hunks)pkg/types/types.go(1 hunks)
🔇 Additional comments (1)
cmd/nuclei/main.go (1)
21-21: LGTM: Clean import addition
The sysutil import is correctly placed and necessary for the new thread management functionality.
Proposed changes
Closes #5605
Checklist
Summary by CodeRabbit
Chores
Refactor