eProsima / Integration-Service

Apache License 2.0
67 stars 28 forks source link

ROS2/WS Handler not populating message array fields correctly. #167

Open ramlab-jose opened 3 years ago

ramlab-jose commented 3 years ago

I am not sure how to reproduce this exactly, but I can leave some logs and brief explanation of what I think is going on. First consider the following: I have a service that allows the client to make a request with set of filenames. This service returns the files and their content or an list with a 0 on it in place of the content if that file does not exist.

The following are two calls done to the same service, the first set through the commandline ros and the second set through a websocket handler of the integration service. Notice how when called through the websocket handler, the result does not match, even though nothing changed. (the requested file names indicate whether or not the file exists, for this example)

requester: making request: ...FileDownload_Request(filename=['exists'])
response:
...FileDownload_Response(file=[...File(name='exists', content=[115, 111, 109, 101, 32, 99, 111, 110, 116, 101, 110, 116, 10], size=13)], success=[True], message=[''])

requester: making request: ...FileDownload_Request(filename=['doesnt_exist'])
response:
...FileDownload_Response(file=[...File(name='', content=[0], size=0)], success=[False], message=['File does not exist'])

[Integration Service][INFO] [is::sh::WebSocket::Server] Handle TCP message from connection '...': [[ {"op": "call_service", "service": ".../file_download", "args": {"filename": ["exists"]}} ]]
[Integration Service][INFO] [is::sh::ROS2] Translating request from Integration Service to ROS 2 for service request topic '.../file_download_Request': [[ Structure: <.../FileDownload:request>
    [filename] <sequence_std::string[1]>
        [0] <std::string>  exists
 ]]
[Integration Service][INFO] [is::sh::WebSocket::Server] Received response from service: [["op":"service_response","result":true,"service":".../file_download","values":{"file":[{"content":[115,111,109,101,32,99,111,110,116,101,110,116,10],"name":"exists","size":13}],"message":[""],"success":[true]}} ]]

[Integration Service][INFO] [is::sh::WebSocket::Server] Handle TCP message from connection '...': [[ {"op": "call_service", "service": ".../file_download", "args": {"filename": ["doesnt_exist"]}} ]]
[Integration Service][INFO] [is::sh::ROS2] Translating request from Integration Service to ROS 2 for service request topic '.../file_download_Request': [[ Structure: <.../FileDownload:request>
    [filename] <sequence_std::string[1]>
        [0] <std::string>  doesnt_exist
 ]]
[Integration Service][INFO] [is::sh::WebSocket::Server] Received response from service: [[ {"op":"service_response","result":true,"service":".../file_download","values":{"file":[{"content":[0,111,109,101,32,99,111,110,116,101,110,116,10],"name":"","size":0}],"message":["File does not exist"],"success":[false]}} ]]

Notice that the file content for the file that does not exist (in the websocket version) comes populated with the same content as the previous message, but with the first element replaced (which should be the only element there). At first I thought I was doing something wrong with buffers but the fact that the ros service call through the command line works as expected makes it clear there is something going wrong with the handlers.

If the example/explanation I gave is not sufficient, I'm happy to make a minimal example, but this would take me a while to setup and I believe I gave enough information for someone that knows this codebase well to work off of.

EDIT: This only seems to be happening to arrays within nested messages, but I have not extensively tested this yet.

erikschuitema commented 3 years ago

@ramlab-jose and I did some more testing and found that during conversion from a ROS2 service response to xtype via ResizableUnboundedContainerConvert::to_xtype_field() (core/include/is/utils/Convert.hpp), the call to to.resize(N) never downsizes the sequence, it can only increase the sequence size in the internal xtype object that is reused between service responses. This means that after receiving a service response with an array of a certain length, that array will have at least that size in all subsequent service responses that are received and translated. The additional array elements beyond its correct size contain old data.

I can't explain this behavior; not allowing to downsize the sequence seems to be done by design, judging from the documentation of WritableDynamicDataRef::resize() (thirdparty/xtypes/include/xtypes/DynamicData.hpp):

    /// \brief resize the Sequence representing by the DynamicData.
    /// If size is less or equals that the current size, nothing happens,          <-------------
    /// otherwise a default-initialized values are insert to the sequence to increase its size.
    /// \param[int] size New sequence size
    /// \pre The DynamicData must represent a SequenceType.
    /// \pre The bounds must be greater or equal to the new size.
    /// \returns The writable reference to this DynamicData
    WritableDynamicDataRef& resize(
            size_t size)                        // this = SequenceType

We tested the following code change to SequenceInstance::resize() and it seems to solve the issue. But I don't know if there are side effects:

diff --git a/include/xtypes/SequenceInstance.hpp b/include/xtypes/SequenceInstance.hpp
index 848a1de..26f8f2f 100644
--- a/include/xtypes/SequenceInstance.hpp
+++ b/include/xtypes/SequenceInstance.hpp
@@ -152,6 +152,8 @@ public:
     {
         if (size_ >= new_size)
         {
+            // Set new size but don't touch the memory
+            size_ = new_size;
             return;
         }