Fix warnings in tcp/udp UT #2744

JohanBertrand · 2024-05-23T20:25:49Z


*Related Issue(s)*	#2706
*Has Unit Tests (y/n)*
*Documentation Included (y/n)*

Change Description

I am not sure we want to have those changes in the codebase. That could be used as a baseline for a better update.

Most of the warnings in the UT with TCP/UDP module should be fixed with those changes. A mutex is making sure that the receive/open/startup functions are called only if the stop/close function are not going to be called in an other thread.

I added the changes in the unit tests instead of the class itself (in open/close), because I did not find a way to make it work properly.

I also needed to make recv to stop iterating if errno == EAGAIN to avoid a timeout. I'm not sure if that's the proper way to fix this issue.

The only warning still happening for me, after 10.000 tests, is the following (happened only once):

[ RUN      ] Reconnect.ReceiveThreadReconnect
Listening for single client at 127.0.0.1:32769
Connected to 127.0.0.1:32769 as a tcp client
Accepted client at 127.0.0.1:32769
Connected to 127.0.0.1:32769 as a tcp client
Accepted client at 127.0.0.1:32769
Connected to 127.0.0.1:32769 as a tcp client
Accepted client at 127.0.0.1:32769
Accepted client at 127.0.0.1:32769
[WARNING] Failed to recv from port with status -8 and errno 0

I feel like the warnings in the test should make the UT to fail. However, I am not sure how to catch the warning streamed through Fw::Logger::logMsg.

Also fix an error in StringUtils.cpp when compiled on ubuntu20.04 with a FW_ASSERT:

/home/fprime/Fw/Types/StringUtils.cpp:51:40: error: comparison of integer expressions of different signedness: ‘FwSizeType’ {aka ‘long unsigned int’} and ‘long int’ [-Werror=sign-compare]
   51 |     FW_ASSERT((source_size - sub_size) <= std::numeric_limits<FwSignedSizeType>::max());

Fw/Types/StringUtils.cpp

Drv/Ip/IpSocket.cpp

LeStarch · 2024-06-03T20:40:33Z

Drv/Ip/SocketReadTask.cpp

@@ -72,13 +72,15 @@ void SocketReadTask::readTask(void* pointer) {
    SocketIpStatus status = SOCK_SUCCESS;
    SocketReadTask* self = reinterpret_cast<SocketReadTask*>(pointer);
    do {
+        self->m_task_lock.lock();


This makes sense!

LeStarch · 2024-06-03T20:41:08Z

Drv/TcpClient/test/ut/TcpClientTester.cpp

+        if (recv_thread) {
+            //this->component.close();
+            if ((1 + i) == iterations) {
+                this->component.m_task_lock.lock();


These locks should be done in the shutdown and close methods, not just the UT.

I moved the mutex in the close/shutdown functions. I had to update the TcpServerTester.cpp a bit for it to work: the client needs to be closed after the server is closed. I do not have the explanation for this behavior, but otherwise I was getting the following warning:

Connected to 127.0.0.1:57755 as a tcp client Accepted client at 127.0.0.1:57755 [WARNING] Failed to recv from port with status -8 and errno 0

There was a comment stating: // Client must be closed first or the server risks binding to an existing address. Let me know if it is a problem.

I also added the muted on the stop function. Can you confirm that it is something that is expected?

JohanBertrand · 2024-06-04T22:09:44Z

@LeStarch is there something we can do in the UT to check that no warnings are logged using Fw::Logger::logMsg, and cause the UT to fail if it contains warnings?

During my tests, I used that warning messages to realize that something was wrong.

I also used a timeout to make sure that the tests were not taking too long. It was especially useful for checking that the TcpServer tests were working properly. The test could take a few seconds on failure, where it's supposed to be less than 500ms in nominal conditions on my hardware. However, I don't think that's something that can/should be implemented, since it's hardware dependent.

JohanBertrand · 2024-06-04T22:15:53Z

Drv/TcpServer/test/ut/TcpServerTester.cpp

        // Properly stop the client on the last iteration
-        if ((1 + i) == iterations && recv_thread) {
-            this->component.shutdown();


@LeStarch Can you confirm that we do not need this shutdown, since it's already part of the stop function?

Keeping it is causing a timeout on my setup (when the UT is tasking more than 1s)

JohanBertrand · 2024-06-23T21:39:36Z

@LeStarch @thomas-bc Let me know if there is something else to change in this PR

JohanBertrand · 2024-07-19T12:19:10Z

@LeStarch @thomas-bc I don't really understand where the problem is coming from on the Mac OS side and I can not reproduce the problem on my setup. Is it possible for you to have a look at this?

LeStarch · 2024-07-19T17:59:48Z

@JohanBertrand yes we can!

JohanBertrand · 2024-09-01T13:34:14Z

@LeStarch I fixed the merge conflict and merged devel, it should be good to be merged once the issue on Mac OS is resolved

LeStarch · 2024-09-24T17:00:47Z

We have been working to get #2683 merged and are about to do so. In that PR we substantially rearchitect the mutex handling. I will note which changes need to be kept and which are likely superseded.

LeStarch · 2024-09-24T17:02:11Z

Drv/Ip/IpSocket.cpp

@@ -219,8 +219,13 @@ SocketIpStatus IpSocket::recv(U8* data, U32& req_read) {
    for (U32 i = 0; (i < SOCKET_MAX_ITERATIONS) && (size <= 0); i++) {
        // Attempt to recv out data
        size = this->recvProtocol(data, req_read);
+
+        if ((size == -1) && (errno == EAGAIN)) {


@JohanBertrand @csmith608 this file's changes should be kept, as they improve error handling (
EAGAIN is differentiated from EINTR).

LeStarch · 2024-09-24T17:03:04Z

Drv/Ip/SocketReadTask.cpp

@@ -51,34 +51,42 @@ SocketIpStatus SocketReadTask::open() {
 }

 void SocketReadTask::shutdown() {
+    this->m_task_lock.lock();


@JohanBertrand @csmith608 These likely need to be discarded as they affect locking.

LeStarch · 2024-09-24T17:03:26Z

Drv/Ip/SocketReadTask.hpp

@@ -171,7 +171,7 @@ class SocketReadTask {
    Os::Task m_task;
    bool m_reconnect; //!< Force reconnection
    bool m_stop; //!< Stops the task when set to true
-
+    Os::Mutex m_task_lock;


@JohanBertrand @csmith608 These likely need to be discarded as they affect locking.

Drv/TcpClient/test/ut/TcpClientTester.cpp

LeStarch · 2024-09-24T17:04:08Z

Drv/Udp/test/ut/UdpTester.cpp

@@ -106,7 +106,7 @@ void UdpTester::test_with_loop(U32 iterations, bool recv_thread) {
            }
        }
        // Properly stop the client on the last iteration
-        if ((1 + i) == iterations && recv_thread) {
+        if (((1 + i) == iterations) && recv_thread) {


@JohanBertrand @csmith608 keep, code clean-up

LeStarch · 2024-09-24T17:04:15Z

Drv/TcpServer/test/ut/TcpServerTester.cpp

        // Properly stop the client on the last iteration
-        if ((1 + i) == iterations && recv_thread) {
-            this->component.shutdown();
+        if (((1 + i) == iterations) && recv_thread) {


@JohanBertrand @csmith608 keep, code clean-up

LeStarch · 2024-09-24T17:04:33Z

Drv/TcpServer/test/ut/TcpServerTester.cpp

@@ -88,15 +88,16 @@ void TcpServerTester ::test_with_loop(U32 iterations, bool recv_thread) {
                while (not m_spinner) {}
            }
        }
-        client.close(); // Client must be closed first or the server risks binding to an existing address
+


@JohanBertrand @csmith608 Review with upcoming change.

It appears I already changed where this occurs in my latest changes, although I can't remember exactly why. https://github.com/nasa/fprime/blob/devel/Drv/TcpServer/test/ut/TcpServerTester.cpp#L91-L103

I changed the position of client.close() in the last commit, after updating the branch with the changes from devel.

From what I have seen in my different tests, if client.close() is called before this->component.close();, there are some cases where ::accept(serverFd, nullptr, nullptr); in Drv/Ip/TcpServerSocket.cpp (See: https://github.com/nasa/fprime/blob/devel/Drv/Ip/TcpServerSocket.cpp#L110) will hang forever.

It usually happened about once every 1600 CI runs.

I just ran the UT for 100.000 times on my machine with the changes in this PR, without failures on the CI, timeouts or hang.

LeStarch · 2024-09-24T17:04:44Z

Drv/TcpServer/test/ut/TcpServerTester.cpp

            this->component.stop();
            this->component.join();
        } else {
            this->component.close();
        }
+


@JohanBertrand @csmith608 Review with upcoming change.

Same comment about this placement already being changed, so I'm going to leave as is unless someone has thoughts

JohanBertrand · 2024-09-28T18:29:55Z

@LeStarch The branch has been updated with devel.

I can see that the messages logged through Fw::Logger::log do not seem to appear in the test log anymore. Is there a way to print it again?

I used it in this PR for checking that no [WARNING] messages were logged in the test log.

Edit: After manually enabling print in the UT, I am still getting a lot of [WARNING] Failed to recv from port with status -8 and errno 0 messages in TCP client, TCP server and UDP UTs.

Out of curiosity, did you @csmith608 investigate this in your previous PR?

csmith608 · 2024-10-02T01:02:31Z

Hey @JohanBertrand, these unit tests fail on Mac due to the first recv after the send sometimes getting an EAGAIN rather than the actual data. This issue does not appear on Linux.
I added a retry mechanism and a clearer status enum for the EAGAIN situation as the recv man page still classifies this as an error rather than a success.

Drv/TcpClient/TcpClientComponentImpl.cpp

Drv/TcpServer/TcpServerComponentImpl.cpp

Drv/Udp/UdpComponentImpl.cpp

LeStarch · 2024-10-07T21:48:30Z

I am going to merge this, as it seems to correct the UT issues, is what I expect. Any other issues should become a new issue, or PR.

JohanBertrand added 2 commits May 23, 2024 21:06

Fix warnings in TCP/UDP UT

cfcd361

Clean unit tests

07b1e48

LeStarch reviewed Jun 3, 2024

View reviewed changes

Fw/Types/StringUtils.cpp Outdated Show resolved Hide resolved

Drv/Ip/IpSocket.cpp Show resolved Hide resolved

LeStarch reviewed Jun 3, 2024

View reviewed changes

JohanBertrand added 2 commits June 5, 2024 00:03

Fixes from review

078f075

Fixes from review 2

3c45f5c

Revert changes

a825519

JohanBertrand commented Jun 4, 2024

View reviewed changes

TheSkyWalkerGo approved these changes Jul 19, 2024

View reviewed changes

JohanBertrand added 3 commits September 1, 2024 15:29

Merge branch 'devel' into Fix-warnings-in-TCP/UDP-UT

f972a02

Merge branch 'devel' into Fix-warnings-in-TCP/UDP-UT

5bb9668

Fix indentation

ea5b7af

LeStarch reviewed Sep 24, 2024

View reviewed changes

JohanBertrand added 2 commits September 28, 2024 14:21

Merge branch 'devel' into Fix-warnings-in-TCP/UDP-UT

67e3a9d

Fix from merge request

7a319b0

JohanBertrand added 2 commits September 28, 2024 21:06

Fix log format

0105d77

Fix indentation

63e3da3

add another status for no data available and allow retry in UTs

e5b3620

github-advanced-security bot found potential problems Oct 2, 2024

View reviewed changes

LeStarch approved these changes Oct 7, 2024

View reviewed changes

LeStarch merged commit 652c670 into nasa:devel Oct 7, 2024
33 of 34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix warnings in tcp/udp UT #2744

Fix warnings in tcp/udp UT #2744

JohanBertrand commented May 23, 2024 •

edited

Loading

LeStarch Jun 3, 2024

LeStarch Jun 3, 2024

JohanBertrand Jun 4, 2024 •

edited

Loading

JohanBertrand Jun 4, 2024

JohanBertrand commented Jun 4, 2024

JohanBertrand Jun 4, 2024 •

edited

Loading

JohanBertrand commented Jun 23, 2024

JohanBertrand commented Jul 19, 2024

LeStarch commented Jul 19, 2024

JohanBertrand commented Sep 1, 2024

LeStarch commented Sep 24, 2024

LeStarch Sep 24, 2024

LeStarch Sep 24, 2024

LeStarch Sep 24, 2024

LeStarch Sep 24, 2024

LeStarch Sep 24, 2024

LeStarch Sep 24, 2024

csmith608 Sep 27, 2024

JohanBertrand Sep 28, 2024

LeStarch Sep 24, 2024

csmith608 Sep 27, 2024

JohanBertrand commented Sep 28, 2024 •

edited

Loading

csmith608 commented Oct 2, 2024

LeStarch commented Oct 7, 2024

Fix warnings in tcp/udp UT #2744

Fix warnings in tcp/udp UT #2744

Conversation

JohanBertrand commented May 23, 2024 • edited Loading

Change Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanBertrand Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanBertrand commented Jun 4, 2024

JohanBertrand Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

JohanBertrand commented Jun 23, 2024

JohanBertrand commented Jul 19, 2024

LeStarch commented Jul 19, 2024

JohanBertrand commented Sep 1, 2024

LeStarch commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanBertrand commented Sep 28, 2024 • edited Loading

csmith608 commented Oct 2, 2024

LeStarch commented Oct 7, 2024

JohanBertrand commented May 23, 2024 •

edited

Loading

JohanBertrand Jun 4, 2024 •

edited

Loading

JohanBertrand Jun 4, 2024 •

edited

Loading

JohanBertrand commented Sep 28, 2024 •

edited

Loading