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.

Sign up for GitHub

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

Jump to bottom

Configurable network adapters #141

Merged
merged 2 commits into from
Mar 17, 2023
Merged

Configurable network adapters #141

merged 2 commits into from
Mar 17, 2023

Conversation

kgraefe
Copy link
Contributor

@kgraefe kgraefe commented Mar 16, 2023

Relates to #54

@kgraefe kgraefe mentioned this pull request Mar 16, 2023
Closed
@kgraefe kgraefe force-pushed the configurable-network-adapters branch from 4713188 to 6de268f Compare March 16, 2023 08:44
@kgraefe
Copy link
Contributor Author

kgraefe commented Mar 16, 2023

CI fails on building a dependency with error: internal compiler error :-\

@kgraefe kgraefe force-pushed the configurable-network-adapters branch 2 times, most recently from 700da91 to affbb9f Compare March 16, 2023 11:39
@kgraefe
Copy link
Contributor Author

kgraefe commented Mar 16, 2023 •
edited
Loading

CI issue will be resolved with the next nightly (tomorrow?), see rust-lang/rust#109199

@kgraefe kgraefe force-pushed the configurable-network-adapters branch 2 times, most recently from 6d3fc90 to 56e7542 Compare March 16, 2023 13:28
@lemunozm lemunozm self-requested a review March 16, 2023 15:26
@lemunozm lemunozm added the enhancement New feature or request label Mar 16, 2023
lemunozm
lemunozm reviewed Mar 16, 2023
View reviewed changes
Copy link
Owner

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

I really like how the information arrives to the adapters. Very neat! 🚀

Should I expect another PR with the broadcast implementation? Anyways, I like this PR as it is open the door to future implementations with different configurations.

I left some minor comments related to documentation. It should be great an example with its usage, to teach the user how to use it.

src/network.rs Show resolved Hide resolved
src/network.rs Show resolved Hide resolved
src/network.rs Show resolved Hide resolved
src/network/adapter.rs Show resolved Hide resolved
src/network/adapter.rs Show resolved Hide resolved
src/adapters/udp.rs Outdated Show resolved Hide resolved
src/adapters/udp.rs Outdated Show resolved Hide resolved
@kgraefe
Copy link
Contributor Author

kgraefe commented Mar 16, 2023

I wanted to hear your opinion before putting too much effort into it. :-) will work on the details tomorrow.

@lemunozm
Copy link
Owner

Great! 💯

@kgraefe
Copy link
Contributor Author

kgraefe commented Mar 17, 2023 •
edited
Loading

Should I expect another PR with the broadcast implementation?

The implementation is complete. It's actually just socket.set_broadcast(true)? for both connect() and listen(). I included it in this PR so you can see how passing the parameters works.

@kgraefe kgraefe force-pushed the configurable-network-adapters branch from 56e7542 to 14ea4cd Compare March 17, 2023 08:00
@kgraefe
Copy link
Contributor Author

kgraefe commented Mar 17, 2023 •
edited
Loading

I think I addressed all your comments.

I also wrapped the configuration properties in an Option so that we can distinguish the default case from explicitly setting it to false. It doesn't make too much sense for that particular property as the default of bool is false and that's also the default for the sockets broadcast capability. But I think we should establish that as a pattern for all configuration properties.

@kgraefe kgraefe requested a review from lemunozm March 17, 2023 08:10
lemunozm
lemunozm requested changes Mar 17, 2023
View reviewed changes
Copy link
Owner

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

I understand your reason about leaving the broadcast as an option. Nevertheless I think it's difficult for a user to chose between 3 choices when they have 2 effective values true or false: should I choose between None, Some(false) or Some(true) 🤔? None means true or means false for this configuration?

I would prefer these None mapping to a sane default value. And the user could always see what they are in the default() implementation (in case the thing get complex and we need it explicitelly).

src/network.rs Outdated
Comment on lines 129 to 131
/// let config = UdpConnectConfig { broadcast: Some(true)};
/// let addr = "255.255.255.255:7777";
/// let (conn_endpoint, _) = handler.network().connect_with(TransportConnect::Udp(config), addr).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Nice!

src/network/adapter.rs Outdated
/// The [`TransportConnect`] wraps custom transport options for transports that support it. It
/// is guaranteed by the upper level to be of the variant matching the adapter. Therefore other
/// variants can be safely ignored.
/// variant matching
Copy link
Owner

@lemunozm lemunozm Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this line does not fit here: /// variant matching

src/adapters/udp.rs Outdated
) -> io::Result<ConnectionInfo<Self>> {
let config = match config {
TransportConnect::Udp(config) => config,
_ => unreachable!(),
Copy link
Owner

Choose a reason for hiding this comment

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

It's reachable if the user choose an invalid one, right? Maybe a panic()! explaining there was a wrong configuration?

Copy link
Contributor Author

@kgraefe kgraefe Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that can happen? Because if the user chooses a different variant of TransportConnect it will not end up in this adapter.

The user calls NetworkController::connect_with() with the specific TransportConnect struct. In that function the adapter is chosen based on TransportConnect::id() which matches the variant to a Transport variant and returns Trasnport::id() which is then used select the corresponding adapter. So unless you have an bug in the matching in TransportConnect::id() it actually is unreachable. And I think TransportConnect::id() is simple enough to assume it to be bug free.

However, if you prefer a panic!() I wont loose any sleep over it. ;-)

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, ok, you're right! It is fine then to leave the unreachable()!

Copy link
Owner

Choose a reason for hiding this comment

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

I see you have just changed it to panic!(). Whatever solution you choose is ok for me

@kgraefe
Refactor to allow configurable network adapters
2a0f24a 
Signed-off-by: Konrad Gräfe <kgraefe@paktolos.net>
@kgraefe kgraefe force-pushed the configurable-network-adapters branch from 14ea4cd to 894dbd2 Compare March 17, 2023 09:05
@kgraefe kgraefe requested a review from lemunozm March 17, 2023 09:06
@kgraefe
Copy link
Contributor Author

kgraefe commented Mar 17, 2023

sorry, forgot about Option

@kgraefe
udp: Add optional broadcast capabilities
dbdf48d 
Signed-off-by: Konrad Gräfe <kgraefe@paktolos.net>
@kgraefe kgraefe force-pushed the configurable-network-adapters branch from 894dbd2 to dbdf48d Compare March 17, 2023 09:10
lemunozm
lemunozm approved these changes Mar 17, 2023
View reviewed changes
Copy link
Owner

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

Very neat and high quality PR!

@lemunozm lemunozm mentioned this pull request Mar 17, 2023
Closed
3 tasks
@lemunozm lemunozm merged commit bcfa0a0 into lemunozm:master Mar 17, 2023
@kgraefe kgraefe deleted the configurable-network-adapters branch March 17, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lemunozm lemunozm lemunozm approved these changes

Assignees

@kgraefe kgraefe

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kgraefe @lemunozm