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

Add Luv polling functions #130

Merged
merged 1 commit into from
Jan 7, 2022
Merged

Conversation

Sudha247
Copy link
Contributor

@Sudha247 Sudha247 commented Jan 5, 2022

Adds Luv polling functions wrapped in Eio, mainly to be used for Lwt integration.

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 - getting polling working will be very useful for Lwt_eio.

I think you might find it easier to base this code on the Stream module rather than on the File one, as polling in luv seems to be start/stop based rather than one-shot. I haven't used it though, and could be wrong.

Has this code been tested with Lwt_eio? It would be good to add a test case (e.g. in lib_eio_luv/tests/poll.md) to make sure it keeps working.

lib_eio_luv/eio_luv.ml Outdated Show resolved Hide resolved
lib_eio_luv/eio_luv.ml Outdated Show resolved Hide resolved
lib_eio_luv/eio_luv.ml Outdated Show resolved Hide resolved
lib_eio_luv/eio_luv.ml Outdated Show resolved Hide resolved
lib_eio_luv/eio_luv.mli Outdated Show resolved Hide resolved
@Sudha247
Copy link
Contributor Author

Sudha247 commented Jan 6, 2022

Thanks for the useful comments @talex5! I haven't looked at the Stream module, will see if we can base poll on that.

@Sudha247
Copy link
Contributor Author

Sudha247 commented Jan 6, 2022

I've rewritten Poll basing on Stream. Here's a branch of Lwt_eio that works with the Luv backend: https://github.com/Sudha247/lwt_eio/tree/luv. I'll try to add a test for this in eio.

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 - this looks better! I pushed a few minor changes (e.g. fixed indentation with ocp-indent, removed the unused type t, added the type to ignore, formatted it to look more like the rest of the code).

As this isn't directly wrapping the Poll interface (just using it), perhaps it should go somewhere else in the eio_luv.mli file? It might be better to call the functions await_readable etc, for consistency with eio_linux. But that still wouldn't directly allow code to work with both, so we'll still have to think about how to do that.

@talex5 talex5 merged commit 46f8b02 into ocaml-multicore:main Jan 7, 2022
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

2 participants