Potential panic in FuturesUnordered #2529
Comments
gperinazzo
changed the title
|
Was able to make a minimal example. It's pretty unreliable so you may need to run it for a while. Running in debug also seems to trigger the error faster than running in release. use futures::stream::{FuturesUnordered, StreamExt};
async fn run_loop() {
async fn test() {
tokio::spawn(async {
tokio::time::sleep(std::time::Duration::from_millis(1)).await;
})
.await
.unwrap();
}
loop {
let mut futures = FuturesUnordered::new();
for _ in 0..4 {
futures.push(test());
}
let _ = futures.next().await;
drop(futures);
}
}
#[tokio::main]
async fn main() {
run_loop().await
}Here is the backtrace generated when it panics: |
|
I've run the code above for 6 hours on 0.3.17 with no crashes so far. With the 0.3.18 version of the crate it usually crashes within the first 20 minutes. I think it's safe to assume that the issue was introduced by the changes in 0.3.18. |
|
Thanks for the report! Considering that 9080b83 is the only one that changed the code related to FuturesUnordered between 0.3.17 and 0.3.18 (0.3.17...0.3.18), it seems likely to be related to #2493 (cc @ibraheemdev). |
|
That must mean |
|
Could it be |
|
Hm the stack trace shows: |
|
Yes, the debug traceback makes it clear that it's panicking in the The initial backtrace is from a release binary and does not show all calls. It's probably being inlined. |
|
That does make sense, another thread may be holding a reference through that |
|
Okay, I understand the issue now: This comment is thus wrong and you should not call This can be prevented from being mistakenly changed again by making |
|
I've also hit this issue. Since this is a heisenbug that anyone can get through cargo update, I suggest 0.3.18 be yanked and a 0.3.19 cut with a fix. I'd be happy to provide a patch but I'm not sure if a revert #2493 would be accepted or if @ibraheemdev wants try to fix it differently. |
|
(I've yanked futures/futures-util 0.3.18) |
thomasmatecki
added a commit
to thomasmatecki/rg3d
that referenced
this issue
Dec 15, 2021
bors bot
added a commit
to nervosnetwork/ckb
that referenced
this issue
Dec 16, 2021
3243: chore: downgrade futures version r=yangby-cryptape,chanhsu001 a=driftluo ### What problem does this PR solve? ref rust-lang/futures-rs#2529, 0.3.18 has been yank ### Check List Tests - No code ci-runs-only: [ quick_checks,linters ] ### Release note ```release-note None: Exclude this PR from the release note. ``` Co-authored-by: driftluo <driftluo@foxmail.com>
paulip1792
added a commit
to paulip1792/snarkOS
that referenced
this issue
Dec 16, 2021
This was referenced Dec 16, 2021
str4d
added a commit
to str4d/zcash
that referenced
this issue
Dec 16, 2021
In the three days since the last update, `futures 0.3.18` was yanked due to panics: rust-lang/futures-rs#2529
bors bot
added a commit
to probe-rs/probe-rs
that referenced
this issue
Dec 17, 2021
929: Fall back to older version of futures, version 0.3.18 was yanked r=Yatekii a=Tiwalun Revert futures to version 0.3.17 in target-gen, version 0.3.18 was yanked. See rust-lang/futures-rs#2529. Co-authored-by: Dominik Boehi <dominik.boehi@gmail.com>
|
Thank you @taiki-e! |
|
@gperinazzo You are right. I'll re-use this issue to track that problem. EDIT: To clarify, FuturesUnordered will panic to prevent unsoundness, so "unsound" is not strictly correct. |
taiki-e
changed the title
|
Published 0.3.19 & updated title and description of this issue.
|
|
Just to confirm: I was able to trigger this panic with the following code using futures-util 0.3.19. Instead of dropping the FuturesUnordered, it clears it instead. use futures::stream::{FuturesUnordered, StreamExt};
async fn run_loop() {
async fn test() {
tokio::spawn(async {
tokio::time::sleep(std::time::Duration::from_micros(100)).await;
})
.await
.unwrap();
}
let mut futures = FuturesUnordered::new();
loop {
for _ in 0..24 {
futures.push(test());
}
let _ = futures.next().await;
futures.clear();
}
}
#[tokio::main]
async fn main() {
run_loop().await
}Here's the backtrace: |
|
If loop {
// Safety: &mut self guarantees the mutual exclusion `dequeue`
// expects
let task = match unsafe { self.ready_to_run_queue.dequeue() } { |
|
In some cases, while another thread is pushing a into the queue, the The clear method, however, expects no one to be pushing to the queue while it's executing. It'll explicitly panic when that happens, and that is the panic being observed: // SAFETY: We have the guarantee of mutual exclusion required by `dequeue`.
match self.dequeue() {
Dequeue::Empty => break,
Dequeue::Inconsistent => abort("inconsistent in drop"), // <- this is where it's generating the stack traces
Dequeue::Data(ptr) => drop(Arc::from_raw(ptr)),
}Maybe we can remove this requirement from |
|
Another alternative is to just create a new |
SGTM |
|
Hi everyone, just encountered this issue in "0.3.21". Cheers. |
|
Was it while calling clear on FuturesUnordered? If that's the case, you can fix your issue by dropping it and making a new one. |
|
The issue with clear has been fixed in 0.3.30. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
EDIT(taiki-e): The regression that occurred in 0.3.18 has been fixed in 0.3.19, but the underlying problem that 0.3.18 tried to fix has not been fixed yet. This issue has been reused to track that problem as there is a detailed discussion on that problem.
Hey folks,
After upgrading to futures 0.3.18 from 0.3.17, we've noticed our application is panicking under high load. The panic seems to originate in src/stream/futures_unordered/ready_to_run_queue.rs:104 due to the error message
inconsistent in drop:This panic causes a double panic that aborts the application. This seems to only happen when under high load, so it may be a race condition that only happens when there's multiple threads running futures from the
FuturesUnordered. I haven't been able to create a minimal project to reproduce.The text was updated successfully, but these errors were encountered: