Skip to content

Fix hang in nested fixpoint iteration #871

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

Merged
merged 1 commit into from
May 27, 2025

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented May 21, 2025

This fixes a hang in multithreaded fixpoint iteration where the Memo::provisional_retry retried over and over again because all its cycle heads other than itself were finalized at this point.

This also fixes the panic from the kopf repro listed in #831

Copy link

netlify bot commented May 21, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit c24d54f
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/6835ef3248506500082ecf9e

@MichaReiser MichaReiser requested a review from carljm May 21, 2025 09:40
Copy link

codspeed-hq bot commented May 21, 2025

CodSpeed Performance Report

Merging #871 will not alter performance

Comparing MichaReiser:fix-hang (c24d54f) with master (db4c4df)

Summary

✅ 12 untouched benchmarks

@MichaReiser MichaReiser force-pushed the fix-hang branch 3 times, most recently from a6d9b6c to 9f7fa33 Compare May 21, 2025 09:59
tracing::debug!(
"Waiting for {head_index:?} results in a cycle, return {database_key_index:?} once all other cycle heads completed to allow the outer cycle to make progress."
);
hit_cycle = true;
Copy link
Contributor Author

@MichaReiser MichaReiser May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to early-return in #861 but I think that was incorrect. Not early-returning fixes the ty panic for kopf. I might spend some more time to come up with a test case for it but I might also just leave it at it (I already spent an entire week on this :s)

@MichaReiser MichaReiser marked this pull request as ready for review May 21, 2025 10:01
@MichaReiser
Copy link
Contributor Author

This does not fix all occurences of the cycle iteration panic, I can still reproduce it on pandas-stubs

@MichaReiser MichaReiser enabled auto-merge May 27, 2025 17:01
@MichaReiser MichaReiser added this pull request to the merge queue May 27, 2025
Merged via the queue into salsa-rs:master with commit e1fe369 May 27, 2025
12 checks passed
@MichaReiser MichaReiser deleted the fix-hang branch May 27, 2025 17:15
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