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

Eio bindings Zmq #126

Merged
merged 6 commits into from
Feb 13, 2024
Merged

Eio bindings Zmq #126

merged 6 commits into from
Feb 13, 2024

Conversation

andersfugmann
Copy link
Contributor

@andersfugmann andersfugmann commented Jul 4, 2023

Eio aware bindings for zmq.

The changes includes factoring out concurrent bindings (i.e. lwt/async/eio) to allow Eio bindings to follow the same interface.
Commits can be read one by one.

During testing I was puzzled that the Eio tests were significantly slower than the lwt/async tests, however this was due to a fault in the original tests where the delay in sending / receiving was only applied for the first loop iteration. This is also fixed.

@andersfugmann andersfugmann force-pushed the andersfugmann/zmq-eio branch 2 times, most recently from f146b55 to 05393b2 Compare July 4, 2023 20:40
@andersfugmann andersfugmann force-pushed the andersfugmann/zmq-eio branch from 05393b2 to 6df7c25 Compare July 4, 2023 20:45
@andersfugmann
Copy link
Contributor Author

I don't want to just add reviewers, so I kindly ask either @Leonidas-from-XIV or @AndreasDahl to give a review on this.

Copy link
Contributor

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

I am not sure I am the best person to review this, since I don't know very much about Eio but here's at least my observations from the code.

.github/workflows/workflow.yml Outdated Show resolved Hide resolved
}
in
let thread = Eio.Fiber.fork_promise ~sw (fun () ->
Eio.Switch.run (fun sw ->
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the Eio terms here, what does the Switch do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fibers is a lightweight thread. A switch is a collection of fibers and will run until all fibers has ended or cancelled. Daemon fibers will automatically be cancelled once all "regular" fibers has ended.

The construct here is made to control lifetime of the two fibers started. Once the main loop exits, the signalling daemon fiber will be automatically be cancelled. Creating a promise allows the caller of close to wait until the event loop has ended (all handlers completed).

Close is meant to be a nice way to close the socket allowing all readers and writers to end. Its also possible to cancel a switch. If the parent switch (passed as arg this function) is cancelled all child fibers and switches are cancelled.

I hope this explains the construct of the fork -> switch -> fork.

(rule
(alias runtest)
(deps
(:test test.exe))
Copy link
Contributor

Choose a reason for hiding this comment

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

run implicitly creates a dependency on test, so this could be skipped.

Copy link
Contributor Author

@andersfugmann andersfugmann Jul 7, 2023

Choose a reason for hiding this comment

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

I'm not sure I understand. Can the rule be skipped altogether?
Tried to replace the block with

(rule
 (alias runtest)
 (action
  (run test.exe))
 (package zmq-eio))

But that does not work

zmq-eio/test/test.ml Outdated Show resolved Hide resolved
@andersfugmann
Copy link
Contributor Author

Thanks a lot for your comments @Leonidas-from-XIV - Really appreciated!

One question about close semantics. In lwt/async, if the socket is closed any deferreds waiting on either send or recv will never be resolved.

In zmq-eio, close will wait for all current sends and receives. Any requests made after the socket has been closed will raise Closed. I'm considering changing the semantics to raise Closed for all pending requests also when/if the socket is closed. What do you think? What semantics would you expect?

@andersfugmann andersfugmann requested a review from a team as a code owner October 21, 2023 10:25
Copy link
Contributor

@AndreasDahl AndreasDahl left a comment

Choose a reason for hiding this comment

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

Looks good to me

@andersfugmann andersfugmann merged commit afa4d5f into master Feb 13, 2024
@andersfugmann andersfugmann deleted the andersfugmann/zmq-eio branch February 13, 2024 09:27
@andersfugmann andersfugmann mentioned this pull request Mar 14, 2024
andersfugmann added a commit to andersfugmann/opam-repository that referenced this pull request Mar 15, 2024
andersfugmann added a commit to andersfugmann/opam-repository that referenced this pull request Mar 16, 2024
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.

4 participants