Tcp socket improvements

[remi-siffert-ocado]

Couple of improvements related to TCP communication:

1. the error handling in tcp_server::readData seems wrong since the error case should be when the number of bytes received is strictly negative and not equal to 0
2. added early disconnection in tcp_socket::read in case of permanent failure in order to ease failure detection
3. added robustness to UrDriver::sendScript: the program sending is now retried once after closing and reconnecting the secondary stream. This is the workaround described in Headless mode is not working Universal_Robots_ROS_Driver#459. It helps mitigate program sending issues when working with the UR ROS2 driver in headless mode.

New tests added for 2 and 3. For 1 the debug output of TCPServerTest::client_connections seems to confirm the behaviour makes sense. We now see client from FD <xx> sent a connection reset package which is what I'd expect given the client willingly terminates connection.

Before the fix:

DEBUG <xxx>/src/comm/tcp_server.cpp 252: Activity on FD 8 
DEBUG <xxx>/src/comm/tcp_server.cpp 268: 8 disconnected. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 8 failed. 
DEBUG <xxx>/src/comm/tcp_server.cpp 228: Activity on self-pipe 
DEBUG <xxx>/src/comm/tcp_server.cpp 252: Activity on FD 10 
DEBUG <xxx>/src/comm/tcp_server.cpp 268: 10 disconnected. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 8 failed. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 10 failed. 
DEBUG <xxx>/src/comm/tcp_server.cpp 228: Activity on self-pipe 
DEBUG <xxx>/src/comm/tcp_server.cpp 252: Activity on FD 12 
DEBUG <xxx>/src/comm/tcp_server.cpp 268: 12 disconnected. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 8 failed. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 10 failed. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 12 failed. 

After the fix:

DEBUG <xxx>/src/comm/tcp_server.cpp 252: Activity on FD 8 
DEBUG <xxx>/src/comm/tcp_server.cpp 268: 8 disconnected. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 8 failed. 
DEBUG <xxx>/src/comm/tcp_server.cpp 228: Activity on self-pipe 
DEBUG <xxx>/src/comm/tcp_server.cpp 252: Activity on FD 10 
DEBUG <xxx>/src/comm/tcp_server.cpp 303: client from FD 10 sent a connection reset package. 
DEBUG <xxx>/src/comm/tcp_server.cpp 268: 10 disconnected. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 8 failed. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 10 failed. 
DEBUG <xxx>/src/comm/tcp_server.cpp 228: Activity on self-pipe 
DEBUG <xxx>/src/comm/tcp_server.cpp 252: Activity on FD 12 
DEBUG <xxx>/src/comm/tcp_server.cpp 303: client from FD 12 sent a connection reset package. 
DEBUG <xxx>/src/comm/tcp_server.cpp 268: 12 disconnected. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 8 failed. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 10 failed. 
ERROR <xxx>/src/comm/tcp_server.cpp 347: Sending data through socket 12 failed. 

[urfeex]

@remi-siffert-ocado Thanks for catching and addressing this. Is there a reason this is a draft or do you expect a review on this?

[remi-siffert-ocado]

@urfeex It's in draft simply because I was waiting for the pipeline to complete. Seems the failures are not related to my changes though.
Would appreciate some feedback!

[urfeex]

This is looking good overall, thank you very much @remi-siffert-ocado

I have two minor comments, if you could give your opinion, especially regarding the retry-on-failed-send code, that would be great!

[urfeex]

We could also use the reconnectSecondaryStream method you created. Also, the code duplication could be reduced (Though I am not sure whether this would be a better approach):

size_t num_tries = 0;
while( num_tries < 2)
{
  if (secondary_stream_->write(data, len, written))
  {
    URCL_LOG_DEBUG("Sent program to robot:\n%s", program_with_newline.c_str());
    return true;
  }
  URCL_LOG_ERROR("Could not send program to robot");
  if (num_tries == 0)
  {
    URCL_LOG_INFO("Reconnecting secondary stream to retry sending program...");
    reconnectSecondaryStream();
  }
}
URCL_LOG_ERROR("Retry sending program failed!");

[remi-siffert-ocado]

Fair points about code duplication and reusing reconnectSecondaryStream. Probably subjective but I find that having a loop with 2 iterations actually doing different things is not the best when it comes to clarity. I tried an alternate solution in 7639529. How does that look?

[urfeex]

I had the same feeling and also had the thought that using a lambda function would make things prettier. Thank you for your solution, that looks nice!

[urfeex]

This is looking good, thank you very much @remi-siffert-ocado

[urfeex]

The failing CI jobs seem unrelated, hence I will merge this.