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

Panicking in spawned Rayon tasks will abort the process by default. #2409

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

adamreichold
Copy link
Collaborator

C.f. https://docs.rs/rayon/latest/rayon/struct.ThreadPoolBuilder.html#method.panic_handler, i.e. spawn is fire-and-forget and hence cannot propagate the panics anywhere sensible. This leads to test flakiness like https://github.com/quickwit-oss/tantivy/actions/runs/9203311046/job/25314621091.

@fulmicoton
Copy link
Collaborator

@adamreichold thank you for the information! Maybe we should catch panics actually.

@adamreichold
Copy link
Collaborator Author

@adamreichold thank you for the information! Maybe we should catch panics actually.

We could just install a panic handler which logs and moves on, but that could leave the system in an inconsistent state. Debugging any issues resulting from that downstream might not be fun.

Alternatively, we could wrap all closure in catch_unwind and forward the panic payloads as errors, either as thread::Result<crate::Result<Vec<R>> or by adding a panic payload variant to tantivy::Result.

@fulmicoton
Copy link
Collaborator

I was more thinking of catch_unwind

@fulmicoton
Copy link
Collaborator

I am merging this. Let me know if you want to work on the catch_unwind stuff.

@fulmicoton fulmicoton closed this May 29, 2024
@PSeitz
Copy link
Contributor

PSeitz commented Jun 4, 2024

This wasn't merged

@PSeitz PSeitz reopened this Jun 4, 2024
@fulmicoton fulmicoton merged commit 8151925 into main Jun 4, 2024
7 checks passed
@fulmicoton fulmicoton deleted the panic-in-drop branch June 4, 2024 08:04
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.

None yet

3 participants