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

Extract the NWConnection code into StateManagedNWConnectionChannel #174

Merged

Conversation

Joannis
Copy link
Contributor

@Joannis Joannis commented May 21, 2023

Extract the NWConnection code into StateManagedNWConnectionChannel

Motivation:

When implementing #68, a lot of code ended up being identical between TCP and UDP. This PR paves the way to adding UDP, with very little overhead code. It moves the code that is used to represent NWConnection as a Channel into its own protocol, that can then be applied to both TCP and UDP implementations.

Modifications:

  • Added StateManagedNWConnectionChannel that has all the logic for handling a NWConnection as a NIO.Channel.
  • Added a new internal protocol NWOptionsProtocol that can read and write socket options. This is applied to NWProtocolTCP.Option
    • StateManagedNWConnectionChannel handles the common denominators between TCP and the future UDP implementaiton
  • Added NWConnectionSubstate that can handle changes in the ChannelState.

Result:

This makes implementing UDP much simpler, and without redundant code.

@Joannis
Copy link
Contributor Author

Joannis commented May 21, 2023

@Lukasa

@Joannis
Copy link
Contributor Author

Joannis commented May 21, 2023

In hindsight, I do think it's valuable to also add a common implementation for listener channels, and add that to UDP right away as well. Give me a bit more time.

EDIT: It'll be a second PR.

@Joannis Joannis marked this pull request as draft May 21, 2023 09:11
This was referenced May 21, 2023
@Joannis Joannis marked this pull request as ready for review May 21, 2023 19:21
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label May 24, 2023
@Joannis
Copy link
Contributor Author

Joannis commented May 24, 2023

Updated now! I removed the lock from addressCache and moved it back into the getters and setters accessing the cache

…the underscores to the addressCache properties
@Joannis
Copy link
Contributor Author

Joannis commented May 26, 2023

Updated @Lukasa , thanks for the quick reviews thus far. And have a nice weekend for now :)

@Joannis Joannis requested a review from Lukasa June 12, 2023 07:13
@Joannis
Copy link
Contributor Author

Joannis commented Jun 21, 2023

Ping @Lukasa

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Ok great, this LGTM.

@Lukasa
Copy link
Contributor

Lukasa commented Jul 8, 2023

@swift-server-bot add to allowlist

@Lukasa
Copy link
Contributor

Lukasa commented Jul 8, 2023

To try to get this landed I took the liberty of just fixing the soundness violation myself.

@Lukasa Lukasa merged commit 7a18352 into apple:main Jul 8, 2023
@Joannis Joannis deleted the feature/jo/StateManagedNWConnectionChannel branch December 13, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants