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

Added startup, shutdown methods to IpSocket and added startup as a re… #2261

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

LeStarch
Copy link
Collaborator

…try step

Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

Briefly, fixes the TcpServer such that it will retry both the listen and accept steps not just the accept step.

More in-depth:

Sockets originally were implemented with open and close steps. However, some sockets (like the TcpServer) needed to have startup and showdown steps too. This was patched in directly to this socket, which meant that retries on generic sockets didn't handle this case.

This PR adds:

  1. startup and shutdown methods generically to all sockets (with the default being no-op)
  2. isStarted method to check status
  3. Retry start up in the read loop if not started.

Drv/Ip/IpSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/IpSocket.hpp Dismissed Show dismissed Hide dismissed
Drv/Ip/TcpServerSocket.cpp Fixed Show fixed Hide fixed
Drv/Ip/IpSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/SocketReadTask.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/TcpServerSocket.cpp Fixed Show resolved Hide resolved
Drv/Ip/SocketReadTask.cpp Fixed Show resolved Hide resolved
Drv/Ip/TcpServerSocket.cpp Fixed Show resolved Hide resolved
Copy link
Collaborator Author

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Possible issues:

  1. Inconsistent use of this->getSocketHandler()->[[ action ]] and this->[[ action]] in SocketReadTask
  2. No check for isStarted in socket read thread loop

}

SocketIpStatus TcpServerSocket::openProtocol(NATIVE_INT_TYPE& fd) {
NATIVE_INT_TYPE clientFd = -1;
NATIVE_INT_TYPE serverFd = -1;

Check notice

Code scanning / CodeQL

Use of basic integral type Note

serverFd uses the basic integral type int rather than a typedef with size and signedness.
@LeStarch
Copy link
Collaborator Author

This code works. Tested with:

nc -l 50000
fprime-gds --ip-client

Then killed nc. Waited for socket wait then the server socket reconnected!

@LeStarch LeStarch added the Update Instructions Needed Need to add instructions in the release notes for updates. label Aug 29, 2023
Copy link
Collaborator

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

Looks good to me!
Couple of notes:

  • We're going to want to remove the explicit calls to startup/shutdown in the fprime-tools cookiecutters - also in references/tutorials if we switched any to be TcpServers already?
  • Browsing through the code while reviewing this, I noticed that all references to Berkeley sockets in the codebase are misspelled (missing an "e" in Berkeley, e.g. here). This PR could be a good opportunity to fix that with a quick search/replace?

@thomas-bc
Copy link
Collaborator

REMINDER

Please merge nasa/fprime-tools#168 after this has been merged

@LeStarch LeStarch merged commit 5e0b388 into nasa:devel Oct 24, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Update Instructions Needed Need to add instructions in the release notes for updates.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants