ros-industrial / industrial_core

ROS-Industrial core communication packages (http://wiki.ros.org/industrial_core)
156 stars 181 forks source link

tcp server makeConnect() blocks forever #177

Open jrgnicho opened 7 years ago

jrgnicho commented 7 years ago

In the absence of a connection, the TcpServer::makeConnect() method blocks the main thread and it won't exit even when the program is terminated with Ctrl-C. I can get around this by placing calling makeConnect() from a second thread however this alternative feels unsafe as the an unhandled exception is thrown when the program exits. I ran my node in ubuntu 16.04 with ros-kinetic.

gavanderhoorn commented 7 years ago

One way to fix this is to put this line in a while with a select(..) and only accept(..) whenever select(..) returns instead of timing out (this has a slight chance of introducing a race-condition, so should be carefully tested).

jrgnicho commented 7 years ago

@gavanderhoorn I tried using the select(...) approach you had suggested but it would error out even when a client had already connected, I followed the manual here for guidance on the proper use of select

What worked for me was to add the following lines around the accept() call

    // disable blocking
    int opts = fcntl(this->getSrvrHandle(),F_GETFL,0);
    opts |= O_NONBLOCK;
    fcntl(this->getSrvrHandle(), F_SETFL, opts);

    // accepting connection
    rc = ACCEPT(this->getSrvrHandle(), NULL, NULL);

    // re-enable blocking
    opts &= ~O_NONBLOCK;
    fcnlt(this->getSrvrHandle(), F_SETFL, opts);

If this solution is desirable I could submit a PR however some of the unit tests did fail after making this change.

shaun-edwards commented 7 years ago

@jrgnicho, this goes beyond my knowledge of the socket library. How did you come up with this solution? Is there a reference somewhere?

Can you provide a test for this? I'm not entirely sure how you issue a Ctrl+C programmatically, but it might be possible.

jrgnicho commented 7 years ago

Here is a link that describes how to disable blocking. However, I decided not to use that approach since it caused some of the unit tests in simple message to fail. My final solution was to call makeConnect() from a second thread and then call shutdown on the socket handle from the main thread. Perhaps we can add a close method to TcpServer that just calls shutdown on the server handle.