Skip to content

Adding initial TCP server traits #30

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

Merged

Conversation

ryan-summers
Copy link
Member

@ryan-summers ryan-summers commented Nov 22, 2020

This PR refactors the TCP traits to align with #21.

This PR fixes #27 by adding a new TcpServer trait.

This PR fixes #16 by clarifying the life cycle of a TCP socket. (socket() -> connect() -> close()).

This PR fixes #10 by updating connect() to return a nb::Result so that connect can be completed asynchronously.

TODO:

  • Implement a test use case of a TCP server using std::net for demonstration purposes.

CC @MathiasKoch @chrysn

Copy link
Contributor

@jonahd-g jonahd-g left a comment

Choose a reason for hiding this comment

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

I don't know much about the TCP side of things. Is it possible for a single socket to communicate with multiple clients at once? Or would you need multiple sockets listening on the same port? Or something else?

@ryan-summers
Copy link
Member Author

@jonahd-g

I don't know much about the TCP side of things. Is it possible for a single socket to communicate with multiple clients at once?

With a TCP server, you typically have a single socket listening on a given local port. It then receives connection requests from various remote clients and creates a new connection for each request. Each connection is assigned a unique socket for the communication stream, but the original listening socket remains operational to facilitate more connections. Thus, a single TCP server can serve any number of TCP clients that it wants to support simultaneously.

Or would you need multiple sockets listening on the same port? Or something else?

Based on my reading of berkley-sockets:

accept() is used on the server side. It accepts a received incoming attempt to create a new TCP connection from the remote client, and creates a new socket associated with the socket address pair of this connection.

There is a single listening socket that creates new sockets for each accepted connection.

@jonahd-g
Copy link
Contributor

There is a single listening socket that creates new sockets for each accepted connection.

That makes sense, thanks for explaining.

Copy link
Collaborator

@MathiasKoch MathiasKoch left a comment

Choose a reason for hiding this comment

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

LGTM, once the stuff from @jonahd-g has been resolved 👍

Great work!

@ryan-summers
Copy link
Member Author

ryan-summers commented Nov 27, 2020

I have now written a small TCP echo server using the traits to verify that everything works as I would expect to - there was one modification to update accept() to return the connected client's address, but other than that, everything worked nicely. This should be ready for merge I believe @MathiasKoch, @Sympatron and @jonahd-g.

If we'd like to add type-state control of the TCP listening socket, I would advocate we wait for another PR. There's some difficulties because of the open() and close() APIs having to get re-implemented.

@MathiasKoch
Copy link
Collaborator

MathiasKoch commented Nov 30, 2020

If we'd like to add type-state control of the TCP listening socket, I would advocate we wait for another PR. There's some difficulties because of the open() and close() APIs having to get re-implemented.

I agree on this, as it seems like a super nice to have approach, but kind of out-of-reach at this point, unless one of you guys have the time to draft up such an API?

So from my side of things, LGTM

@jonahd-g
Copy link
Contributor

I'm willing to start working on a typestate solution, but I'd really like to see this current approach merged and released in the mean-time, considering the currently-released version does not support binding.

@Sympatron
Copy link
Contributor

LGTM. I think we can merge it the way it is and add type states in a new PR with a separate discussion.

@ryan-summers
Copy link
Member Author

CC: @eldruin - this should now be ready for merge

@eldruin
Copy link
Member

eldruin commented Dec 1, 2020

Thanks @ryan-summers! GitHub complains about conflicts, though. Could you rebase this?

@ryan-summers
Copy link
Member Author

Alright, should now be fixed up after a rebase (there was indeed a conflict)

@eldruin eldruin merged commit 3a67f58 into rust-embedded-community:master Dec 2, 2020
@eldruin
Copy link
Member

eldruin commented Dec 2, 2020

Thanks everyone!

eldruin added a commit to eldruin/embedded-nal that referenced this pull request Dec 2, 2020
eldruin added a commit that referenced this pull request Dec 2, 2020
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.

Add support for TCP server APIs Confusing TCP connection states Support for asynchronous TCP connections
5 participants