Add Open Port Request on Sending Side of IpSocket #2683
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
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. |
Drv/Ip/SocketReadTask.cpp
Outdated
| // 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.
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.
Line 98 of your file contains the original open that we should replace with this code
|
@LeStarch I added a unit test to TcpClient and TcpServer to show the socket auto opening, so I think this should be okay to merge (if it passes the checks). |
|
Better way of doing this:
|
csmith608
force-pushed
the
dev-ip-socket
branch
from
aa38100
to
b3d8dbb
Compare
…oved behavior to the component level by placing thread protection behavior to SocketReadTask, now called SocketComponentHelper. Calls to socket functionality from TcpClient/TcpServer/Udp will go through SocketComponentHelper rather than directly to the library. This also implemented the original desired functionality of reopening a client socket on a send call rather than only in receive
csmith608
force-pushed
the
dev-ip-socket
branch
from
b3d8dbb
to
baebba1
Compare
| } | ||
| // As long as not told to stop, and we are successful interrupted or ordered to retry, keep receiving | ||
| while (not self->m_stop && | ||
| (status == SOCK_SUCCESS || status == SOCK_INTERRUPTED_TRY_AGAIN || self->m_reconnect)); |
Check warning
Code scanning / CppCheck
Empty loop bodies should use {} or continue Warning
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…tests whos name includes more than the module name
…buffer equality check would fail because the size of the original buffer (owned by the unit test) gets set to 0, sometimes. Maybe 1/10 runs. I changed it to save the size of that buffer before it does the send and use that saved value in the equality check
| return status; | ||
| } | ||
|
|
||
| SocketIpStatus IpSocket::send(const U8* const data, const U32 size) { | ||
| SocketIpStatus IpSocket::send(NATIVE_INT_TYPE fd, const U8* const data, const U32 size) { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| @@ -205,27 +160,19 @@ | |||
| return SOCK_SUCCESS; | |||
| } | |||
|
|
|||
| SocketIpStatus IpSocket::recv(U8* data, U32& req_read) { | |||
| SocketIpStatus IpSocket::recv(NATIVE_INT_TYPE fd, U8* data, U32& req_read) { | |||
Check notice
Code scanning / CodeQL
Long function without assertion Note
|
|
||
| namespace Drv { | ||
|
|
||
| SocketComponentHelper::SocketComponentHelper() : m_fd(-1), m_reconnect(false), m_stop(false), m_started(false), m_open(false) {} |
Check notice
Code scanning / CodeQL
More than one statement per line Note
|
|
||
| SocketComponentHelper::SocketComponentHelper() : m_fd(-1), m_reconnect(false), m_stop(false), m_started(false), m_open(false) {} | ||
|
|
||
| SocketComponentHelper::~SocketComponentHelper() {} |
Check notice
Code scanning / CodeQL
More than one statement per line Note
|
|
||
| SocketComponentHelper::~SocketComponentHelper() {} | ||
|
|
||
| void SocketComponentHelper::start(const Fw::StringBase &name, |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| this->shutdown(); // Break out of any receives and fully shutdown | ||
| } | ||
|
|
||
| SocketIpStatus SocketComponentHelper::recv(U8* data, U32 &size) { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| return status; | ||
| } | ||
|
|
||
| void SocketComponentHelper::readTask(void* pointer) { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| @@ -24,7 +24,7 @@ | |||
|
|
|||
| TcpClientComponentImpl::TcpClientComponentImpl(const char* const compName) | |||
| : TcpClientComponentBase(compName), | |||
| SocketReadTask() {} | |||
| SocketComponentHelper() {} | |||
Check notice
Code scanning / CodeQL
More than one statement per line Note
| @@ -22,13 +22,14 @@ | |||
|
|
|||
| TcpServerComponentImpl::TcpServerComponentImpl(const char* const compName) | |||
| : TcpServerComponentBase(compName), | |||
| SocketReadTask() {} | |||
| SocketComponentHelper() {} | |||
Check notice
Code scanning / CodeQL
More than one statement per line Note
| @@ -24,16 +24,22 @@ | |||
|
|
|||
| UdpComponentImpl::UdpComponentImpl(const char* const compName) | |||
| : UdpComponentBase(compName), | |||
| SocketReadTask() {} | |||
| SocketComponentHelper() {} | |||
Check notice
Code scanning / CodeQL
More than one statement per line Note
|
@LeStarch other than the RPi known issue, I think this PR is ready for review |
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)