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

Fix Condition.await bug when cancelling #487

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

talex5
Copy link
Collaborator

@talex5 talex5 commented Apr 14, 2023

Condition.await needs to re-acquire the lock when cancelled. However, if another fiber is holding the lock then this operation must wait and would itself get cancelled.

Reported by @polytypic.

It's a little annoying that we have to do a non-cancellable wait during cancellation, since typically we only need the lock so we can call unlock on it again immediately. I did try adding a wait_until function that handles all the locking, which allows simplifying this. However, we would need multiple versions of it to cover the various ways of using a lock (use_rw, use_ro, and ~protected), which seems a bit messy.

`Condition.await` tries to re-acquire the lock when cancelled. However,
if another fiber is holding the lock then this operation must wait and
would itself get cancelled.

Reported by Vesa Karvonen <vesa.a.j.k@gmail.com>
@talex5 talex5 added the bug Something isn't working label Apr 14, 2023
@talex5 talex5 requested a review from polytypic April 14, 2023 09:57
@polytypic
Copy link
Contributor

The fix seems correct. On quick look the failures don't seem related?

@talex5
Copy link
Collaborator Author

talex5 commented Apr 14, 2023

Yes, [WARNING] Failed to allocate 4096 byte fixed buffer means the build machine was a bit overloaded. Normally Eio would fall back to using a regular buffer, but that test is specifically for checking that the fixed buffer works.

@talex5 talex5 merged commit 9656b89 into ocaml-multicore:main Apr 14, 2023
@talex5 talex5 deleted the fix-cond branch April 14, 2023 15:17
@talex5 talex5 added this to the 0.10 milestone May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants