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

Add TCP server and client classes #34

Merged
merged 3 commits into from
May 30, 2023
Merged

Add TCP server and client classes #34

merged 3 commits into from
May 30, 2023

Conversation

domire8
Copy link
Member

@domire8 domire8 commented May 8, 2023

Description

Again, a slightly large PR due to the definition of three new classes. The TCPSocket class itself is also pure virtual because TCP communication is a communication-based protocol, hence between sending/receiving, we need a connect step. For the server, this is a blocking operation (see docstrings). Apart from the fact that connection needs to be established with a few extra steps, the TCP implementation is very similar to UDP.

Review guidelines

Estimated Time of Review: 15 minutes

@domire8 domire8 requested a review from eeberhard May 8, 2023 14:19
sockaddr_in new_socket_address{};
socklen_t new_addr_len = sizeof(new_socket_address);
// accept, create a new socket descriptor to handle the new connection with client
this->socket_fd_ = accept(this->server_fd_, (sockaddr*) &new_socket_address, &new_addr_len);
Copy link
Member Author

Choose a reason for hiding this comment

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

What's happening here is that the socket file descriptor that is used for communication is not the same as the socket that was opened and used for accepting connections. Instead, we create a new socket for the first incoming request from a client.

@domire8 domire8 linked an issue May 14, 2023 that may be closed by this pull request
* @brief Connect the TCP socket to its counterpart (client / server)
* @details This method should be called after `open()` and may be blocking (for the case of a server)
*/
virtual void connect() = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This does not respect the interface defined in ISocket. Would it be appropriate just to call connect() at the end of open()? Based on the simple test case it seems it would suffice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I guess we could do it like that as well, but then open() becomes blocking for the tcp server

@domire8
Copy link
Member Author

domire8 commented May 17, 2023

Putting this on hold for now

@domire8
Copy link
Member Author

domire8 commented May 25, 2023

This is now active again but the comment above about doing the connection in open was not addressed yet. From how I read online, it is "normal" that there is this extra step in TCP communication as it is a connection-based protocol (unlike UDP and ZMQ). I guess it would be fair to make the connection on open but then again, this will become a blocking function.

@eeberhard
Copy link
Member

Could you see if there is a connect_timeout configuration option we can pass? I don't see the problem in open() being blocking if that's the behavior of the specific socket, as long as it can be aborted after some connection timeout. I expect controllers and other users will try to open the connection on configure or similar, where it's reasonable to assume that the respective client / server on the other end should be open and ready, else configuration fails.

@domire8
Copy link
Member Author

domire8 commented May 30, 2023

Could you see if there is a connect_timeout configuration option we can pass?

There isn't. accept doesn't accept other arguments that would allow to set a timeout (same in Python btw).

I expect controllers and other users will try to open the connection on configure or similar, where it's reasonable to assume that the respective client / server on the other end should be open and ready, else configuration fails.

Yes, but I expect controllers to create tcp clients, not servers. The server always has to be up before the client. So if we assume that we will most often use clients and only rarely create servers ourselves, I can just move the code from connect to open if you prefer.

@eeberhard
Copy link
Member

I can just move the code from connect to open if you prefer

Let's do it like that for now, to keep a consistent interface, and then see if the blocking is an issue in practice.

@domire8 domire8 merged commit 97ec448 into develop May 30, 2023
@domire8 domire8 deleted the feature/tcp branch May 30, 2023 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement TCP sockets
2 participants