Skip to content

Conversation

@malloch
Copy link
Contributor

@malloch malloch commented Oct 17, 2025

This PR includes 2 fixes:

  1. after a socket was removed due to POLLERR or POLLHUP, the revents for the next socket were being cleared, since after lo_server_del_socket() is called the socket index i now refers to the next socket (if it exists). Socket memory is not reallocated here so no memory-access error occured.
  2. Partial receipt of a message when using TCP was causing lo_server_recv() to return early, even though no message had been dispatched. This PR ensured that lo_server_recv() keeps looping in this case.

return dispatch_queued(s, 0);
while (1) {
while ((ret = lo_servers_wait_internal(&s, &recvd, &queued, 1, 100)) == 0) {}
if (ret > 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

Okay I'm really happy with this patch in the sense that I didn't expect it would be so simple. But it does feel scary introducing a while (1) here, no exit plan at all in case of problems. (Although admittedly the existing inner while is arguably just as scary.)

However, I think what's happened here is that lo_server_recv is simply the oldest "recv" function and doesn't have the same semantics in the return values as lo_server_wait for instance, where its docs indicate that it may return "-1 if there is an error that causes the function to return early". Unfortunately lo_server_recv has no such error code.

(Also according to docs, the combination of lo_server_wait and lo_server_recv is supposed to guarantee reception of a single message, so I guess this was the "out" I tried to design in post-hoc regarding this situation.)

But, here we see that ret can have the value -1 due to lo_servers_wait_internal. And there is the question if we should really keep looping on poll if poll returned an error, which is what makes it return that.

So before accepting this change, I want to just consider here whether there is any danger of backwards-incompatible breakage if we exit this while if ret == -1 (or ret < 0) Then, do we return the -1 from lo_server_recv and add it to the docs, or do we return 0, which would be closer to the current behaviour?

Co-authored-by: Stephen Sinclair <radarsat1@users.noreply.github.com>
@malloch
Copy link
Contributor Author

malloch commented Oct 21, 2025

Github seems to have deleted my comment :(

In brief it suggested:

  • use of lo_server_recv() should be discouraged for new projects
  • we should try to maintain the previous behaviour of lo_server_recv() to avoid breaking old projects. This is more important than matching the current documentation, and means returning zero if there is a failure of some sort, rather than my previous suggestion that it should loop forever until a valid message is dispatched
  • @radarsat1's suggestions are great and I accepted them

We still need to deal with an edge case if lo_server_recv_internal() returns an error but there are valid messages in the queue. In this case I think we should dispatch a queued message rather than returning an error. The only change needed is to line 1629:

        if (recvd && (ret = lo_server_recv_internal(s)) > 0) {

@radarsat1 radarsat1 merged commit 2469ca2 into radarsat1:master Oct 24, 2025
4 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