[malloch]

Another fix for handling multiple servers in the poll() codepath – if a TCP socket is closed the num_sockets value changes and iteration through the array of poll results is broken, possibly leading to a hang if a POLLHUP is handled for a UDP socket and lo_server_recv_internal() waits for data that does not exist.

Apologies for not catching this along with the prior fix in 90c05b3. I think this fix warrants another liblo release – hope you agree!

[radarsat1]

This loop is pretty darn confusing to reason about.

I've been trying to figure out it could be made clearer by constructing sockets in the reverse order, perhaps paired with index data, instead of iterating over it backwards, but no good solution comes to mind yet that doesn't just move the problem elsewhere.

[radarsat1]

This change seems unrelated and I'm not 100% sure if it's correct. Don't we also want to catch POLLERR here? I'm not quite sure, to be honest. Perhaps we're just missing handling for POLLERR in the UDP case? Or is it that it can't happen? I understand that POLLHUP can't happen but POLLERR, I am not sure. It might be nice to add. Not sure what to do in case anyway, close the socket?

According to this there is some action to take: https://stackoverflow.com/questions/35468555/poll-handling-with-udp

But maybe this warrants a different commit or PR.

[malloch]

The problem this line addresses was exposed due to the erroneous increment so a TCP socket's revents were being handled by the UDP code block, but in any case we should not be marking recvd=1 for this socket unless revents==POLLIN since it will cause a hang in lo_server_recv_internal(). It seemed safer to test for POLLIN explicitly instead of assuming that revents!=0 means POLLIN.

Perhaps we can add a TODO stub and handle UDP+POLLERR later in a separate commit? From the link you posted it seems like a rare edge case (hardware error?).

diff --git a/src/server.c b/src/server.c
index b45d672..838acd9 100644
--- a/src/server.c
+++ b/src/server.c
@@ -1473,6 +1473,9 @@ int lo_servers_wait_internal(lo_server *s, int *recvd, int *queued, int num_serv
                     s[j]->sockets[0].revents = POLLIN;
                     recvd[j] = 1;
                 }
+                else if (POLLERR & sockets[k].revents) {
+                    /* TODO: handle hardware error by destroying and recreating this socket */
+                }
                 ++k;
             }
         }

[radarsat1]

Since this only accesses sockets_len once, I assume the real problem is "k += sockets_len" on line 1469. In that case it would be clearer to move "int sockets_len" initialization to right before this line, with a comment "make a local copy since lo_server_del_socket modifies s[j]->sockets_len, but we need it for resetting k after walking backwards"

[malloch]

We are using it to iterate over the sockets array allocated on line 1402, so we need sockets_len to match the number of sockets that existed when poll() was called rather than the current number as sockets are added/removed from the TCP server. I think the initialization needs stay on line 1424. 100% agree on adding a comment to explain the reason for storing the value.

[radarsat1]

merging and i'll add a couple of commits on top