Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Estimator panic #403

Merged
merged 1 commit into from
Mar 17, 2022
Merged

Fix Estimator panic #403

merged 1 commit into from
Mar 17, 2022

Conversation

arxanas
Copy link
Contributor

@arxanas arxanas commented Mar 17, 2022

This is happening in some of my CI runs, like here: https://github.com/arxanas/git-branchless/runs/5581044721?check_suite_focus=true

@arxanas
Estimator: Fix panic when rewinding position
3dc989a 
In some situations, we can get a panic like this:

```
The application panicked (crashed).
Message:  attempt to subtract with overflow
Location: /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/indicatif-0.17.0-rc.9/src/state.rs:339
```

This could happen if we set the progress of a meter to a lower value than it was previously. This commit fixes the panic that would happen in such cases.
@arxanas arxanas marked this pull request as ready for review March 17, 2022 08:18
arxanas added a commit to arxanas/git-branchless that referenced this pull request Mar 17, 2022
@arxanas
fix(effects): work around upstream indicatif panic
2c46f99 
The upstream issue is fixed by console-rs/indicatif#403.
@chris-laplante
Copy link
Collaborator

FWIW I have also seen weird panics in places arithmetic is being done on the Instants. For me it was in AtomicPosition::allow. I have not figured out a way to reproduce it reliably otherwise I'd have reported it. I feel like we should be using saturating_add and saturating_sub everywhere as a precaution though.

@djc djc merged commit 653b849 into console-rs:main Mar 17, 2022
@djc
Copy link
Member

djc commented Mar 17, 2022

LGTM, thanks for adding a test!

arxanas added a commit to arxanas/git-branchless that referenced this pull request Mar 17, 2022
@arxanas
fix(effects): work around upstream indicatif panic
a838fa6 
The upstream issue is fixed by console-rs/indicatif#403.
@arxanas arxanas deleted the estimator-panic branch March 17, 2022 19:20
@arxanas
Copy link
Contributor Author

arxanas commented Mar 18, 2022

I've also been seeing panics in AtomicPosition::allow, but I also wasn't able to reproduce them.

@djc
Copy link
Member

djc commented Mar 18, 2022

I think #404 might fix that.

@arxanas arxanas mentioned this pull request Mar 18, 2022
Closed
@arxanas
Copy link
Contributor Author

arxanas commented Mar 18, 2022

I filed #405 to track the AtomicPosition::allow panic. I think it's not addressed in #404; it's probably overflowing because of the as u8 somehow, which is still present there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arxanas @chris-laplante @djc