fix(sort): GNU sort-continue.sh test #9107
Merged
+70
−17
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…mits - Add `effective_merge_batch_size()` function to calculate batch size considering fd soft limit, with minimums and safety margins. - Generalize fd limit handling from Linux-only to Unix systems using `fd_soft_limit()`. - Update merge logic to use dynamic batch size instead of fixed `settings.merge_batch_size` to prevent fd exhaustion.
…dling Replace direct call to get_rlimit()? with fd_soft_limit(), adding a check for None value to return a usage error if rlimit cannot be fetched. This improves robustness on Linux by ensuring proper error handling when retrieving the file descriptor soft limit.
CodSpeed Performance ReportMerging #9107 will not alter performanceComparing Summary
Footnotes
|
|
GNU testsuite comparison: |
mattsu2020
changed the title
Contributor
|
@mattsu2020 hey, thanks for the PR. to fix the docs related issues, you'll need to escape the brackets as in the error log. The rest of the CI fails are unrelated. |
|
GNU testsuite comparison: |
Contributor
Author
This has been improved in another commit. |
Contributor
|
@sylvestre can you restart the CI, this seems ready to merge :) |
|
GNU testsuite comparison: |
Contributor
|
@mattsu2020 RLIM_INFINITY doesn't seem to be available there. |
Update conditional compilation attributes from #[cfg(unix)] to #[cfg(target_os = "linux")] for the nix::libc import and fd_soft_limit function implementations, ensuring these features are only enabled on Linux systems to improve portability and avoid issues on other Unix-like platforms.
Contributor
Author
Modified to restrict it to Linux |
|
GNU testsuite comparison: |
sylvestre
reviewed
Dec 19, 2025
| Ok(()) | ||
| } | ||
|
|
||
| fn effective_merge_batch_size(settings: &GlobalSettings) -> usize { |
sylvestre
reviewed
Dec 19, 2025
src/uu/sort/src/sort.rs
Outdated
| 0 => Ok(limit.rlim_cur as usize), | ||
| _ => Err(UUsageError::new(2, translate!("sort-failed-fetch-rlimit"))), | ||
|
|
||
| let result = unsafe { libc::getrlimit(libc::RLIMIT_NOFILE, &raw mut limit) }; |
Contributor
There was a problem hiding this comment.
please use use nix::sys::resource::getrlimit;
to avoid the unsafe
Replace unsafe libc::getrlimit calls in fd_soft_limit with safe nix crate usage. Update Rayon thread configuration to use ThreadPoolBuilder instead of environment variables for better control. Add documentation comment to effective_merge_batch_size function for clarity.
Extract the rlimit fetching logic into a separate `get_rlimit` function that returns `UResult<usize>` and properly handles errors with `UUsageError`, instead of silently returning `None` on failure or infinity. This provides better error reporting for resource limit issues on Linux platforms.
Reordered the nix::sys::resource imports to group constants first (RLIM_INFINITY), then types (Resource), and finally functions (getrlimit), improving code readability and adhering to import style guidelines.
|
GNU testsuite comparison: |
oech3
pushed a commit
to oech3/coreutils
that referenced
this pull request
Dec 20, 2025
* feat: dynamically adjust merge batch size based on file descriptor limits - Add `effective_merge_batch_size()` function to calculate batch size considering fd soft limit, with minimums and safety margins. - Generalize fd limit handling from Linux-only to Unix systems using `fd_soft_limit()`. - Update merge logic to use dynamic batch size instead of fixed `settings.merge_batch_size` to prevent fd exhaustion. * fix(sort): update rlimit fetching to use fd_soft_limit with error handling Replace direct call to get_rlimit()? with fd_soft_limit(), adding a check for None value to return a usage error if rlimit cannot be fetched. This improves robustness on Linux by ensuring proper error handling when retrieving the file descriptor soft limit. * refactor(sort): restrict nix::libc and fd_soft_limit to Linux Update conditional compilation attributes from #[cfg(unix)] to #[cfg(target_os = "linux")] for the nix::libc import and fd_soft_limit function implementations, ensuring these features are only enabled on Linux systems to improve portability and avoid issues on other Unix-like platforms. * refactor: improve thread management and replace unsafe libc calls Replace unsafe libc::getrlimit calls in fd_soft_limit with safe nix crate usage. Update Rayon thread configuration to use ThreadPoolBuilder instead of environment variables for better control. Add documentation comment to effective_merge_batch_size function for clarity. * refactor(linux): improve error handling in fd_soft_limit function Extract the rlimit fetching logic into a separate `get_rlimit` function that returns `UResult<usize>` and properly handles errors with `UUsageError`, instead of silently returning `None` on failure or infinity. This provides better error reporting for resource limit issues on Linux platforms. * refactor(sort): reorder imports in get_rlimit for consistency Reordered the nix::sys::resource imports to group constants first (RLIM_INFINITY), then types (Resource), and finally functions (getrlimit), improving code readability and adhering to import style guidelines.
CrazyRoka
pushed a commit
to CrazyRoka/coreutils
that referenced
this pull request
Dec 28, 2025
* feat: dynamically adjust merge batch size based on file descriptor limits - Add `effective_merge_batch_size()` function to calculate batch size considering fd soft limit, with minimums and safety margins. - Generalize fd limit handling from Linux-only to Unix systems using `fd_soft_limit()`. - Update merge logic to use dynamic batch size instead of fixed `settings.merge_batch_size` to prevent fd exhaustion. * fix(sort): update rlimit fetching to use fd_soft_limit with error handling Replace direct call to get_rlimit()? with fd_soft_limit(), adding a check for None value to return a usage error if rlimit cannot be fetched. This improves robustness on Linux by ensuring proper error handling when retrieving the file descriptor soft limit. * refactor(sort): restrict nix::libc and fd_soft_limit to Linux Update conditional compilation attributes from #[cfg(unix)] to #[cfg(target_os = "linux")] for the nix::libc import and fd_soft_limit function implementations, ensuring these features are only enabled on Linux systems to improve portability and avoid issues on other Unix-like platforms. * refactor: improve thread management and replace unsafe libc calls Replace unsafe libc::getrlimit calls in fd_soft_limit with safe nix crate usage. Update Rayon thread configuration to use ThreadPoolBuilder instead of environment variables for better control. Add documentation comment to effective_merge_batch_size function for clarity. * refactor(linux): improve error handling in fd_soft_limit function Extract the rlimit fetching logic into a separate `get_rlimit` function that returns `UResult<usize>` and properly handles errors with `UUsageError`, instead of silently returning `None` on failure or infinity. This provides better error reporting for resource limit issues on Linux platforms. * refactor(sort): reorder imports in get_rlimit for consistency Reordered the nix::sys::resource imports to group constants first (RLIM_INFINITY), then types (Resource), and finally functions (getrlimit), improving code readability and adhering to import style guidelines.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adjust fd_soft_limit helper to expose the soft RLIMIT for reuse across the codebase.
Respect the soft limit when determining merge batch sizes so the external merge never exceeds the available file descriptors.
Fallback to a useful error message when retrieving the limit fails while parsing --batch-size.
Tests:
cargo build -p uu_sort
ulimit -n 7; ./target/debug/sort -n -m __test.* … (GNU sort-continue scenario)
ulimit -n 7; ./target/debug/sort -n -m __test.* - …