Yarp: Yarpserver get stuck if it reads a string with a single "

[traversaro]

At vvv18, one team had several different problems when a xml application file was malformed because it contained a " in the port name in the connection tag:
~xml

"/module/port1
/module/port2
tcp

~
or in dependencies tag:
~xml

"/module/port1

~

The problems seems to be both at the yarpmanager level ( that was freezing ) @Nicogene and at the yarpserver level, that was behaving in a strange way when queryed with a port name like "/module/port1 @drdanz . A possible solution could be to sanitize port names loaded from the application file, as soon as possible.

cc @marco-monforte @atabakd @JuanMiguelAlvarez @ericpairet @anqingd @raedbsili1991 please provide any additional details that could be useful to avoid this problem in the future, thanks!

[Nicogene]

Somehow related to #1508. I think we should sanitize the port names in general 😅
Thanks for reporting, I will investigate why yarpmanager freeze !
The freeze happens when you try to connect the malformed port ??

[traversaro]

I will let the students answer this, in particular @marco-monforte and @anqingd that work in the iCub Facility.

[ericpairet]

Hi @traversaro @Nicogene. I had a look at the history of our cursed .xml file. Here goes the snippet:

<connection>
     <from>/orange/vision/controller:o</from>
     <to>"/orange/vision/controller:i"</to>
     <protocol>tcp</protocol>
</connection>

Unfortunately, we only saw one of the two " during our last time slot, and by then we run out of time to test it agian. That's why in the demo, yarpmanager freezed again, with only one ".

If I don't remember incorrectly, yarpmanager was freezing as soon as the modules in the app were run. I guess that this behaviour might be straightforward to reproduce at least, with the code in our repository. Apart from the " issue, there is the remote chance that something else was contributing to the freeze.

[marco-monforte]

Hi guys and sorry for replying only now, but I traveled all the day yesterday.

Anyway, as @ericpairet said, the problem was the apexes in the port name. This should be easily repeatable from one of the last commits (or maybe even from the last, if Eric didn't change the error 😜). We can have a look together at IIT on Monday or as soon as you are available.

[Nicogene]

My hope was that it was a yarpmanager problem, but I'm afraid that it is deeper 😔
yarpmanager before connecting checks that the ports exist calling NetworkBase::exists(...)
If you call:

yard::os::NetworkBase::exists("\"notExistingPort")

it get stuck.

So I would move this issue as a libYARP_OS issue, what do you think about @drdanz @traversaro ?

[Nicogene]

Digging into the YARP core I found out that the problem is that the NameClient sends a query to the nameserver and waits that yarpserver answer back.
On the other side the yarpserver get stuck in yarp::name::NameServerConnectionHandler::read, in particular here:

        yarp::os::Bottle cmd, reply, event;
        bool ok = cmd.read(reader);

where reader is a ConnectionReader...

I think that the problem is in the parsing in BottleImpl::read() :thinking:

[traversaro]

[Nicogene]

Probably I found the solution of the mystery :male_detective:

BottleImpl::read() calls bool BottleImpl::isComplete(char* txt) and if the string read from the connection reader is not complete he tries to read the rest from the socket.

In this case the string with only one " is considered as not complete so it waits the rest of the string.

The only way to fix it is to check the inputs before sending requests to the yarpserver, I don't think that the parsing of the bottle is bugged(in this case :stuck_out_tongue:) the string is correctly marked as incomplete

[drdanz]

Does yarpserver get stuck if it receives such a string?
Sending commands through YARP is not the only way to send commands to yarpserver, so checking the input before sending the request is a good practice, but the server should not be in the "incomplete" state forever, perhaps a timeout would help

[Nicogene]

With @damn1 and @drdanz we found a possible solution:
if you put

    delegate->getInputStream().setReadTimeout(1.0);
    delegate->getOutputStream().setWriteTimeout(1.0);

inside yarp::os::impl::NameserTwoWayStream, all the requests to yarpserver will have a timeout of 1.0 seconds.

In this way if you put only one " the string will be marked as incomplete, and the nameserver will wait only one second for the rest of the message, and then if it doesn't receive anything it will close the connection.

With this fix yarp exists \" doesn't get stuck but it have this error message that it is quite misleading:

yarp: No connection to nameserver
yarp: *** try running: yarp detect ***

Any suggestions for better fixes ?
We could find a way to print an error message more appropriate :thinking:

[pattacini]

Doable for me @Nicogene 👍

Anyway, is there any room for unexpected behaviors?

In other words, is there at the moment any case where the connection may stay freezed for slightly more than 1 second and then restored soon afterward (e.g. very overloaded network due to complicated demos)?

[drdanz]

I suggest to add a "--timeout" option (setting default 1.0) to yarpserver

[pattacini]

The command line option is worthy, but I would make up a more meaningful name than just timeout.
As for the default, I'd be less strict; something like 2 or more seconds.

[lornat75]

uhm, timeouts lead to the dark side...

[pattacini]

Timeouts are an anti-pattern I know, but sometimes they're the only way out.

[lornat75]

Setting a unique timeout on the yarpserver may be a limitation, but we could do it and see if it works out.