avoid deadlock when reporting ice

[SparrowLii]

Fixes the deadlock issue in #110284

When using the parallel compiler, we have added deadlock headler via rayon's thread pool, but outside the thread pool (printing the query stack when reporting ice) it is still possible to stuck into deadlocks. So I added a timeout to print_query_stack to get away from deadlocks.

The impl of print_query_stack is transferred from rustc_query_system to rustc_query_impl because we need to use with_context to enable the sub thread to access TLV

cc @Zoxc

[rustbot]

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

[cjgillot]

Can try_collect_active_jobs be removed from the QueryContext trait, and be made an inherent impl?

[SparrowLii]

I think we need to keep it, since rustc_query_system needs it to handle cycle errors:
https://github.com/rust-lang/rust/blob/master/compiler/rustc_query_system/src/query/plumbing.rs#L269

[cjgillot]

Could you add a comment explaining why we spawn a thread here?

[SparrowLii]

Done :)

[cjgillot]

Should this be an inherent method on QueryCtxt?

[SparrowLii]

Sure, done : )

[SparrowLii]

Edit: I changed to use rayon_core::join now so no need to use with/enter_context. Reverted the change to lift up print_query_stack.

[Zoxc]

Calling try_collect_active_jobs on an outside thread can run into issues with WorkerLocal.

[SparrowLii]

I think reporting ice is an action outside the thread pool, so this is not a regression?

[SparrowLii]

I used rayon_core::join instead of spawn so that there is no problem with WorkerLocal.

But I still have a bit doubts—— if reporting ice is running in rayon's thread pool, why the deadlock handler is not performed

[Zoxc]

How does the actual deadlock occur?

I think running the deadlock handler on a Rayon thread as a Rayon job may be an improvement, but I'm not sure that would help for the actual deadlock you ran into.

[SparrowLii]

How does the actual deadlock occur?

Not sure yet. try_collect_active_job theoretically should not be deadlocked, since it only calls try_lock(). So I guess it has something to do with the specific bug for the derive use case(#110284).
Anyway, I think adding a timeout here is the easiest way to allow us to continue with other more important work.

[pnkfelix]

I think @Zoxc's request for a description of the actual deadlock is important here. Its really important to understand the root causes of problems like that, if you can isolate them, in order to confirm that the proposed fix is actually a real fix and not just masking it.

retagging as waiting-on-author to account for the need for an answer to that question.

@rustbot label: +S-waiting-on-author -S-waiting-on-review

[SparrowLii]

OK, I gonna figure out the real cause of the deadlock.

[SparrowLii]

It looks like the problem is with make_query called in try_collect_active_jobs.

I experimented like this in try_collect_active_jobs:

    pub fn try_collect_active_jobs<Qcx: Copy>(
        &self,
        qcx: Qcx,
        make_query: fn(Qcx, K) -> QueryStackFrame<D>,
        jobs: &mut QueryMap<D>,
    ) -> Option<()> {
        #[cfg(parallel_compiler)]
        {
            // We use try_lock_shards here since we are called from the
            // deadlock handler, and this shouldn't be locked.
            let locks = self.active.locks();
            for lock in locks.iter() {
                use std::sync::mpsc::channel;
                use std::time::Duration;

                let timeout = Duration::from_secs(5); // Set the timeout to 5 seconds

                let (tx, rx) = channel();
                let map = lock.try_lock()?;

                std::thread::spawn(move || {
                    match rx.recv_timeout(timeout) {
                        Ok(_) => (),
                        Err(_) => eprintln!("collect actives failed: time out"),
                    }
                });
                for (k, v) in map.iter() {
                    if let QueryResult::Started(ref job) = *v {
                        let query = make_query(qcx, *k);
                        jobs.insert(job.id, QueryJobInfo { query, job: job.clone() });
                    }
                }
                tx.send(()).unwrap();
            }
        }
        #[cfg(not(parallel_compiler))]
        {
            // We use try_lock here since we are called from the
            // deadlock handler, and this shouldn't be locked.
            // (FIXME: Is this relevant for non-parallel compilers? It doesn't
            // really hurt much.)
            for (k, v) in self.active.try_lock()?.iter() {
                if let QueryResult::Started(ref job) = *v {
                    let query = make_query(qcx, *k);
                    jobs.insert(job.id, QueryJobInfo { query, job: job.clone() });
                }
            }
        }

        Some(())
    }

And it printed:

query stack during panic:
collect actives failed: time out
print query stack failed: time out

[bors]

☔ The latest upstream changes (presumably #108714) made this pull request unmergeable. Please resolve the merge conflicts.

[SparrowLii]

close this as #112708 has solved the issue