Skip to content

Prioritize returning Fiber.any value over cancelling quickly#806

Merged
talex5 merged 1 commit intoocaml-multicore:mainfrom
dialohq:main
Aug 11, 2025
Merged

Prioritize returning Fiber.any value over cancelling quickly#806
talex5 merged 1 commit intoocaml-multicore:mainfrom
dialohq:main

Conversation

@adamchol
Copy link
Contributor

This makes changes in Fiber.any to prioritize returning the value rather than cancelling the whole Fiber.any from the outside if we already have the value from one of it's functions. A sample code showing a specific scenario and more detailed explanation are in the issue #805.

Let me know if this is reasonable to merge.

@talex5
Copy link
Collaborator

talex5 commented Jun 13, 2025

The tests need updating too (CI is failing).

@adamchol
Copy link
Contributor Author

I changed the test from fiber.md and the description of it to showcase the priority on returning the value.

I'm not sure about the rest of the tests in the CI. One of them is new I think and I don't know how the rest is related to the code I modified. Tried to reproduce those locally but unsuccessfully because of some environment problems with docker. I might need some help with the rest of those CI tests.

@adamchol
Copy link
Contributor Author

Hi again! Can I get some help with the remaining tests in CI, please? I would really benefit from this PR being merged.

@talex5
Copy link
Collaborator

talex5 commented Jul 4, 2025

Rebasing on main should fix most of the CI problems (see #808). The ERROR while compiling mdx.2.5.0 on OCaml 5.4-alpha1 is due to realworldocaml/mdx#469 and can be ignored.

@adamchol
Copy link
Contributor Author

Thanks! Done!

@adamchol
Copy link
Contributor Author

adamchol commented Jul 28, 2025

Hi! Can this be merged now, please?

@talex5
Copy link
Collaborator

talex5 commented Aug 8, 2025

I've rebased it and squashed (so there are no commits in the history with failing tests). I slightly modified the test to check that the parent cancellation takes effect on the next yield.

I don't have permission to update your branch (possibly because it's on main), so I've pushed the commit here: https://github.com/talex5/eio/pull/new/fiber-any-keep-result

If you reset your branch to that version I'm happy to merge it - thanks!

Originally, `Fiber.any` didn't have a `combine` argument, so it was
always expected that it was OK to throw away either result, and aborting
early made sense. But with `combine` that's no longer true.
@adamchol
Copy link
Contributor Author

Done!

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Thanks!

@talex5 talex5 merged commit 62b9714 into ocaml-multicore:main Aug 11, 2025
5 checks passed
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

Comments