Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warnings in tcp/udp UT #2744

Merged
merged 13 commits into from
Oct 7, 2024
7 changes: 6 additions & 1 deletion Drv/Ip/IpSocket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,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);

LeStarch marked this conversation as resolved.
Show resolved Hide resolved
if ((size == -1) && (errno == EAGAIN)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

req_read = 0;
return SOCK_SUCCESS;
}
// Error is EINTR, just try again
if (size == -1 && ((errno == EINTR) || errno == EAGAIN)) {
if ((size == -1) && (errno == EINTR)) {
continue;
}
// Zero bytes read reset or bad ef means we've disconnected
Expand Down
10 changes: 10 additions & 0 deletions Drv/Ip/SocketReadTask.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,34 +51,42 @@ SocketIpStatus SocketReadTask::open() {
}

void SocketReadTask::shutdown() {
this->m_task_lock.lock();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this->getSocketHandler().shutdown();
this->m_task_lock.unlock();
}

void SocketReadTask::close() {
this->m_task_lock.lock();
this->getSocketHandler().close();
this->m_task_lock.unlock();
}

Os::Task::Status SocketReadTask::join() {
return m_task.join();
}

void SocketReadTask::stop() {
this->m_task_lock.lock();
this->m_stop = true;
this->getSocketHandler().shutdown(); // Break out of any receives and fully shutdown
this->m_task_lock.unlock();
}

void SocketReadTask::readTask(void* pointer) {
FW_ASSERT(pointer);
SocketIpStatus status = SOCK_SUCCESS;
SocketReadTask* self = reinterpret_cast<SocketReadTask*>(pointer);
do {
self->m_task_lock.lock();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense!

// Open a network connection if it has not already been open
if ((not self->getSocketHandler().isStarted()) and (not self->m_stop) and
((status = self->startup()) != SOCK_SUCCESS)) {
Fw::Logger::logMsg(
"[WARNING] Failed to open port with status %d and errno %d\n",
static_cast<POINTER_CAST>(status),
static_cast<POINTER_CAST>(errno));
self->m_task_lock.unlock();
(void) Os::Task::delay(SOCKET_RETRY_INTERVAL);
continue;
}
Expand All @@ -90,6 +98,7 @@ void SocketReadTask::readTask(void* pointer) {
"[WARNING] Failed to open port with status %d and errno %d\n",
static_cast<POINTER_CAST>(status),
static_cast<POINTER_CAST>(errno));
self->m_task_lock.unlock();
(void) Os::Task::delay(SOCKET_RETRY_INTERVAL);
continue;
}
Expand All @@ -113,6 +122,7 @@ void SocketReadTask::readTask(void* pointer) {
}
self->sendBuffer(buffer, status);
}
self->m_task_lock.unlock();
}
// As long as not told to stop, and we are successful interrupted or ordered to retry, keep receiving
while (not self->m_stop &&
Expand Down
2 changes: 1 addition & 1 deletion Drv/Ip/SocketReadTask.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
}
#endif // DRV_SOCKETREADTASK_HPP
3 changes: 2 additions & 1 deletion Drv/TcpClient/test/ut/TcpClientTester.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,13 @@ void TcpClientTester ::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) {
LeStarch marked this conversation as resolved.
Show resolved Hide resolved
this->component.stop();
this->component.join();
} else {
this->component.close();
}

server.close();
}
server.shutdown();
Expand Down
7 changes: 4 additions & 3 deletions Drv/TcpServer/test/ut/TcpServerTester.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohanBertrand @csmith608 Review with upcoming change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@JohanBertrand JohanBertrand Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

if (((1 + i) == iterations) && recv_thread) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohanBertrand @csmith608 keep, code clean-up

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohanBertrand @csmith608 Review with upcoming change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

client.close();
}
ASSERT_from_ready_SIZE(iterations);
}
Expand Down
6 changes: 3 additions & 3 deletions Drv/Udp/test/ut/UdpTester.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,9 @@ void UdpTester::test_with_loop(U32 iterations, bool recv_thread) {
}
}
// Properly stop the client on the last iteration
if ((1 + i) == iterations && recv_thread) {
this->component.stop();
this->component.join();
if (((1 + i) == iterations) && recv_thread) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohanBertrand @csmith608 keep, code clean-up

this->component.stop();
this->component.join();
} else {
this->component.close();
}
Expand Down
2 changes: 1 addition & 1 deletion Fw/Types/StringUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ FwSignedSizeType Fw::StringUtils::substring_find(const CHAR* source_string,
return -1;
}
// Confirm that the output type can hold the range of valid results
FW_ASSERT((source_size - sub_size) <= std::numeric_limits<FwSignedSizeType>::max());
FW_ASSERT(source_size - sub_size <= static_cast<FwSizeType>(std::numeric_limits<FwSignedSizeType>::max()));

// Loop from zero to source_size - sub_size (inclusive)
for (FwSizeType source_index = 0;
Expand Down
Loading