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 send_all on eio #132

Merged
merged 3 commits into from
May 19, 2024
Merged

Fix send_all on eio #132

merged 3 commits into from
May 19, 2024

Conversation

ComanderP
Copy link
Contributor

Was trying the bindings on eio and for some reason Zmq_eio.Socket.send_all wasn't working at all, but Zmq_eio.Socket.send was. Checked the source code and I suppose the wrong argument was given inside the function (do correct me if I'm wrong).
It finally works with the change locally built on my machine (at least I think so).

@ComanderP ComanderP requested a review from a team as a code owner May 5, 2024 22:56
Copy link
Contributor

@andersfugmann andersfugmann left a comment

Choose a reason for hiding this comment

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

The messages argument is important, as the send_all should be fully completed (i.e. all messages sent) before continuing.

zmq-eio/src/socket.ml Outdated Show resolved Hide resolved
zmq-eio/src/socket.ml Outdated Show resolved Hide resolved
@andersfugmann
Copy link
Contributor

andersfugmann commented May 7, 2024

That definitely seems like an error - but I think the fix is incomplete. Thanks for raising this!

The messages to be sent needs to happen as part of the function. I've added suggestions (although I'm not sure they work as expected). Also I think the tests needed to be extended to verify send_all and recv_all.

@ComanderP
Copy link
Contributor Author

ComanderP commented May 7, 2024

I've added your suggestions to the pull request.
For the tests, I'm not sure if I'm the right person to do them 😅, but I can surely try if someone gives me some guidance.

(Sorry for the force push, was amending the commit message)

@andersfugmann
Copy link
Contributor

Tests should be added to eio_zmq/test/test.ml. A simple test that verifies that send_all followed by a receive_all works as expected. Look at test_send_receive. However that test uses the send and recv functions defined in the test, which in terms calls Zmq_eio.Socket.send, so you will need to call Zmq_eio.Socket.send_all and Zmq_eio.Socket.recv_all function directly.

@ComanderP
Copy link
Contributor Author

Done.
There was a repeated usage of the 1st send/receive test test_send_receive which I replaced with the new test (I assume that was a typo).

@andersfugmann
Copy link
Contributor

Looks good. Thanks for doing this. I'll merge once the CI tests passes

@andersfugmann andersfugmann merged commit 614de51 into issuu:master May 19, 2024
3 checks passed
@andersfugmann
Copy link
Contributor

Thanks

@ComanderP ComanderP deleted the eio-fix branch July 9, 2024 22:45
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