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

channel: Panic in .recv_timeout(Duration::MAX) #795

Closed
MightyPork opened this issue Mar 3, 2022 · 7 comments · Fixed by #798
Closed

channel: Panic in .recv_timeout(Duration::MAX) #795

MightyPork opened this issue Mar 3, 2022 · 7 comments · Fixed by #798

Comments

@MightyPork
Copy link

MightyPork commented Mar 3, 2022

https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-channel/src/channel.rs#L863

Calling recv_timeout() with Duration::MAX causes panic due to Instant overflow.

This happens quite naturally when the duration comes from some calculation, meaning "wait forever".

I don't think it should crash, insted wait the longest possible time.

@taiki-e
Copy link
Member

taiki-e commented Mar 4, 2022

It would be ideal if there were a method like Instant::saturating_add, but unfortunately it does not exist, so for now it seems we have to wait a few decades or wait forever.

@MightyPork
Copy link
Author

There's checked add, so I'm thinking something like

self.recv_deadline(match Instant::now().checked_add(timeout) {
    Some(deadline) => deadline,
    None => Instant::now() + Duration::from_secs(86400 * 365 * 30) // hack from tokio-rs
})

@taiki-e
Copy link
Member

taiki-e commented Mar 4, 2022

Sounds good to me.

bors bot added a commit that referenced this issue Mar 15, 2022
798: channel: Do not panic on very large timeout r=taiki-e a=taiki-e

Fixes #795

Co-authored-by: Taiki Endo <[email protected]>
@bors bors bot closed this as completed in 8efcd02 Mar 15, 2022
@taiki-e
Copy link
Member

taiki-e commented Mar 15, 2022

Fixed in crossbeam-channel 0.5.3.

@MightyPork
Copy link
Author

I was planning to make a PR xD

thanks for fixing it, this was quite the footgun 👍

@ibraheemdev
Copy link
Contributor

I fixed this in the PR to std with:

    pub fn recv_timeout(&self, timeout: Duration) -> Result<T, RecvTimeoutError> {
-        self.recv_deadline(Instant::now() + timeout)
+        match Instant::now().checked_add(timeout) {
+            Some(deadline) => self.recv_deadline(deadline),
+            // So far in the future that it's practically the same as waiting indefinitely.
+            None => self.recv().map_err(RecvTimeoutError::from),
+        }
    }

@taiki-e
Copy link
Member

taiki-e commented Mar 17, 2022

@ibraheemdev I agree that is also reasonable approach.

bors bot added a commit that referenced this issue Jan 19, 2023
953: Wait forever on very large timeout r=ibraheemdev a=taiki-e

Adopts the same approach as the standard library: #795 (comment)

r? `@ibraheemdev`
bors d=ibraheemdev

Co-authored-by: Taiki Endo <[email protected]>
bors bot added a commit that referenced this issue Jan 19, 2023
953: Wait forever on very large timeout r=ibraheemdev a=taiki-e

Adopts the same approach as the standard library: #795 (comment)

r? `@ibraheemdev`
bors d=ibraheemdev

Co-authored-by: Taiki Endo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants