Fix #5437: 修复总任务数为 0 时 Task 进度计算错误的问题#5442
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #5437 which addresses a task progress calculation error when the total task count is 0. When dividing by zero, the result is NaN, which previously would cause an IllegalArgumentException to be thrown. The fix handles this and other edge cases by clamping invalid progress values.
Changes:
- Modified
updateProgress(long count, long total)to handle edge cases including negative values, values greater than 1.0, and NaN (which occurs when total is 0) - Renamed the first parameter from
progresstocountfor better clarity - Updated the error message in
updateProgress(double)to include the actual invalid progress value
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (progress < 0) { | ||
| updateProgress(0); | ||
| } else if (progress > 1.0 || Double.isNaN(progress)) { | ||
| updateProgress(1.0); | ||
| } else { | ||
| updateProgress(progress); | ||
| } |
There was a problem hiding this comment.
The error handling is now inconsistent between the two updateProgress overloads. The updateProgress(long, long) method silently clamps invalid values (negative, greater than 1.0, or NaN), while updateProgress(double) throws an IllegalArgumentException for the same invalid values. This means if you call updateProgress(5, 3) (which results in progress = 1.67), it will be silently clamped to 1.0, but if you directly call updateProgress(1.67), it will throw an exception. Consider whether both methods should have consistent error handling behavior - either both throw exceptions for invalid inputs, or both silently clamp values.
| if (progress < 0) { | |
| updateProgress(0); | |
| } else if (progress > 1.0 || Double.isNaN(progress)) { | |
| updateProgress(1.0); | |
| } else { | |
| updateProgress(progress); | |
| } | |
| updateProgress(progress); |
| double progress = (double) count / total; | ||
| if (progress < 0) { | ||
| updateProgress(0); | ||
| } else if (progress > 1.0 || Double.isNaN(progress)) { |
There was a problem hiding this comment.
When count is greater than total (e.g., updateProgress(5, 3)), the result is clamped to 1.0. While this prevents the exception, it silently hides what might be a bug in the calling code. Consider whether this scenario indicates a programming error that should be reported (perhaps with a warning log) rather than silently corrected. This could help catch bugs where the count or total values are incorrectly calculated.
| } else if (progress > 1.0 || Double.isNaN(progress)) { | |
| } else if (progress > 1.0 || Double.isNaN(progress)) { | |
| LOG.warning("updateProgress: computed progress out of range, clamping to 1.0. " | |
| + "count=" + count + ", total=" + total + ", progress=" + progress); |
| protected void updateProgress(long count, long total) { | ||
| double progress = (double) count / total; | ||
| if (progress < 0) { | ||
| updateProgress(0); | ||
| } else if (progress > 1.0 || Double.isNaN(progress)) { | ||
| updateProgress(1.0); | ||
| } else { | ||
| updateProgress(progress); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic has a potential edge case: when both count and total are negative (e.g., updateProgress(-5, -10)), the division results in a positive value (0.5), which would pass all checks and be treated as valid progress. While this scenario is unlikely in practice, it represents invalid input that should be handled. Consider adding explicit validation that total should be non-negative, or that both count and total should be non-negative.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.