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.

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

Avoid panic if run_with_timeout hits timeout. #866

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Avoid panic if run_with_timeout hits timeout. #866

merged 2 commits into from
Sep 13, 2023

Conversation

fiadliel
Copy link
Contributor

@fiadliel fiadliel commented Sep 2, 2023

If run_with_timeout hits the timeout, the mpsc receiver may be dropped, leading to a panic when the background thread later attempts to return its value.

This change simply ignores any error. Since the timeout has already been reached, reporting on this error would just be noise.

A future improvement could be to cancel the background operation, but that would require a much larger refactoring.

Part of #860

If run_with_timeout hits the timeout, the mpsc receiver may be
dropped, leading to a panic when the background thread later
attempts to return its value.

This change simply ignores any error. Since the timeout has
already been reached, reporting on this error would just be
noise.

A future improvement could be to cancel the background operation,
but that would require a much larger refactoring.

Part of #860
@jdx jdx enabled auto-merge (squash) September 13, 2023 13:22
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -3.66% ⚠️

Comparison is base (58b0e0e) 87.45% compared to head (fd0e4d3) 83.80%.
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #866      +/-   ##
==========================================
- Coverage   87.45%   83.80%   -3.66%     
==========================================
  Files         132      132              
  Lines       11578    11579       +1     
==========================================
- Hits        10126     9704     -422     
- Misses       1452     1875     +423     
Files Changed Coverage Δ
src/timeout.rs 100.00% <100.00%> (ø)

... and 22 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jdx jdx disabled auto-merge September 13, 2023 13:38
@jdx jdx merged commit 1e9b871 into jdx:main Sep 13, 2023
5 checks passed
@fiadliel fiadliel deleted the remove_timeout_unwrap branch September 13, 2023 15:14
jdx pushed a commit that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants