-
Couldn't load subscription status.
- Fork 0
Fix multipart sending and receiving #1
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
base: main
Are you sure you want to change the base?
Conversation
8a8786b to
9c5332c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces resumable variants for multipart send/receive operations to handle EINTR/EAGAIN, updates downstream bindings to use them, and adds a test for non-blocking EAGAIN behavior.
- Add
resumabletype and*_rfunctions, deprecating their unsafe counterparts. - Update
zmq-eio,zmq-deferred, andzmq-asyncbindings to use resumable functions and retry on signals. - Add a test for EAGAIN in nonblocking receive and update CHANGES.md.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| zmq/test/zmq_test.ml | Added test_zmq_eagain and updated test suite names to use resumable variants. |
| zmq/src/zmq.mli | Declared 'a resumable and new *_r functions; deprecated old functions. |
| zmq/src/zmq.ml | Implemented *_wrapper_resumable and new *_r functions. |
| zmq-eio/src/socket.ml | Switched to *_r resumable calls and catch EINTR in queue processing. |
| zmq-deferred/src/socket.ml | Updated post to use resumable calls and handle EINTR/EAGAIN. |
| zmq-async/src/deferred.ml | Fixed Async_kernel.Ivar deprecation by using fill_exn. |
| CHANGES.md | Documented new resumable functions, deprecations, and version bump. |
Comments suppressed due to low confidence (9)
zmq/src/zmq.mli:17
- Fix spelling of "corretly" to "correctly".
If EINTR is not handled corretly, the socket may be left with a half written/read
zmq/src/zmq.mli:21
- Remove the duplicated "the the" and correct grammar (e.g., "avoid the socket from going into").
For this reason, if EINTR is raised its important to repeat the the call to the resumable to avoid the the socket go into a broken
zmq-deferred/src/socket.ml:201
- Fix typo in comment: change "Recevie" to "Receive".
(** Recevie all message blocks. *)
CHANGES.md:5
- Correct typo "EAGIN" to "EAGAIN".
* to allow resuming if the calls raise EAGIN or EINTR.
CHANGES.md:4
- [nitpick] Reference the correct function names:
send_msg_all_randrecv_msg_all_rinstead ofsend_all_msg_r/recv_all_msg_r.
* Add `send_all_r`, `send_all_msg_r`, `recv_all_r` and `recv_all_msg_r`
zmq/test/zmq_test.ml:391
- [nitpick] Update test name to "zmq exception EINTR" for clarity and consistency with EAGAIN test.
"zmq exception intr" >:: test_zmq_exception;
zmq/test/zmq_test.ml:392
- [nitpick] Capitalize "EAGAIN" in the test name for consistency.
"zmq exception eagain" >:: test_zmq_eagain;
zmq/src/zmq.ml:502
- [nitpick] Add unit tests covering
recv_all_wrapper_resumableto verify it correctly retries on EINTR/EAGAIN and gathers all parts.
let recv_all_wrapper_resumable: (?block:bool -> _ t -> 'a) -> ?block:bool -> _ t -> 'a list resumable = fun f ?block socket ->
zmq/src/zmq.ml:519
- [nitpick] Add unit tests for
send_all_wrapper_resumableto ensure multipart sends resume correctly after EINTR/EAGAIN.
let send_all_wrapper_resumable: (?block:bool -> ?more:bool -> _ t -> 'a -> unit) -> ?block:bool -> _ t -> 'a list -> unit resumable = fun f ?block socket messages ->
Co-authored-by: Copilot <[email protected]>
|
A little digging on the internet shows that we are not the only ones with problems with multipart messages. See. http://hintjens.com/blog:84. Its interesting that they are were already pondering about deprecating multipart messages back 10 years ago due to the high complexity. Reading that, I'm inclined to simply ignore some of the problems - Modify the core implementation to always retry on EINTR and send all following multipart blocks in blocking mode (So EAGAIN may only be raised from the initial send). |
Interesting read - but concerning this PR my observations are:
Also - I think the nicest API for
|
|
You were mentioning tests of:
What are your thoughts on:
Edit: Edit: |
|
I btw. just reran my application handling
.. and the same behaviour continues - including same kind of |
|
As a sidenote I found this related info page: |
Potential solution to multipart message handling if INTR or EAGAIN is received.
In general, EAGAIN should never be received according to documentation, but it has to be verified. Potentially sending large multipart messages that exceeds local zmq buffers may cause EAGAIN to be raised (or the call to block).
This branch is based on IssuuArchive/ocaml-zmq#135 by @rand00