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
Add Open Port Request on Sending Side of IpSocket #2683
base: devel
Are you sure you want to change the base?
Conversation
@@ -70,7 +70,22 @@ | |||
FW_ASSERT(pointer); | |||
SocketIpStatus status = SOCK_SUCCESS; | |||
SocketReadTask* self = reinterpret_cast<SocketReadTask*>(pointer); | |||
bool disconnected; |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
// Prevent transmission before connection, or after a disconnect | ||
if (this->m_fd == -1) { | ||
SocketIpStatus status; | ||
bool disconnected = false; |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
if (disconnected) { | ||
// comment out so message is not spammed when no IP endpoint exists | ||
// Fw::Logger::logMsg("[WARNING] Failed to open port on read side with status %d and errno %d\n", status, errno); | ||
Os::Task::delay(SOCKET_RETRY_INTERVAL_MS); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
delay
Looks like it passed CI. We'll need to correct spelling. Your spelling is right, so we need to add the word to the expect list. |
The list is at: .github/actions/spelling/expect.txt. Just place "spammed" in there is mostly alphabetical order. I can help this afternoon, but the above shows you how to should you get to it sooner. |
Fixed the spelling issue. Will review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I feel dumb, these comments were left as pending for several weeks.
// Open a network connection if it has not already been open | ||
disconnected = false; | ||
// Lock mutex to avoid competing opens from other threads | ||
self->getSocketHandler().lockSocketMutex(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this code block looks right, I believe it needs to be below (after start-up). I think a start-up needs to be added to to send too (following the same pattern).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 98 of your file contains the original open that we should replace with this code
Change Description
Update IpSocket to check socket state on sending side and open socket if necessary
Rationale
This fixes a bug encountered while using IpSocket. The server would close the TCP connection and then the client would try to send and fail because previously IpSocket would only re-open a TCP connection when it received over the socket.
Testing/Review Recommendations
In my project's version of fprime we diverged at fprime 3.0. I added implementations for the close() port in the TcpClient, TcpServer, and Udp as well as unit tests. This change also fixed the issue we were seeing with sending data, but the socket not being there.
This does not include the unit tests or implementations for close() in the ByteStreamDriverModel because I wanted to at least get this part of the change in. I'll work on adding them.
I also ended up commenting out line 85 of Drv/Ip/SocketReadTask.cpp because when I was running other testing without the TCP server, I was being spammed with that logger message.
Future Work
Adding implementations for close() to TcpClient, TcpServer, and Udp as well as unit tests.
(Pipeline failed on "spammed" but I stand behind my spelling, but perhaps not that comment)