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

feat(swarm): Make executor for connection tasks explicit #3097

Merged
merged 40 commits into from
Nov 15, 2022

Conversation

umgefahren
Copy link
Contributor

@umgefahren umgefahren commented Nov 8, 2022

Description

Previously, the executor for connection tasks silently defaulted to a futures::executor::ThreadPool. This causes issues such as #2230.

With this patch, we force the user to choose, which executor they want to run the connection tasks on which results in overall simpler API with less footguns.

Closes #3068.

Links to any relevant issues

Open Questions

  • Is the API satisfactory?
  • Should the implementation of the executor live in the core, swarm, or somewhere else?
  • What is the correct location for the Swarm aliases

Change checklist

I will add a changelog entry once it's clear how broad the changes are.

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!

Please see inline comments :)

swarm/src/connection/pool.rs Outdated Show resolved Hide resolved
swarm/src/connection/pool.rs Outdated Show resolved Hide resolved
swarm/src/connection/pool.rs Outdated Show resolved Hide resolved
core/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Show resolved Hide resolved
@umgefahren
Copy link
Contributor Author

I can implement these changes, but I have a question: If we go back to trait objects here, which would be fine by me, that means I will remove all the genetics from the APIs it that's what you want. I'm asking because I read your issue a bit differently.

@umgefahren
Copy link
Contributor Author

l implemented the suggested changes. It results in a lot of warnings, but I will remove them if your happy with the api.

@thomaseizinger
Copy link
Contributor

I'm asking because I read your issue a bit differently.

Sorry if that was worded poorly! I tried to list multiple options there because I wasn't sure how things would turn out.

Breaking type inference is definitely a no-no in regards to ergonomics so adding feature-gated functions seems like a better approach!

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks! Some inline comments :)

core/Cargo.toml Outdated Show resolved Hide resolved
core/src/lib.rs Outdated Show resolved Hide resolved
examples/chat.rs Outdated Show resolved Hide resolved
protocols/dcutr/examples/dcutr.rs Outdated Show resolved Hide resolved
swarm/src/connection/pool.rs Outdated Show resolved Hide resolved
swarm/src/connection/pool.rs Outdated Show resolved Hide resolved
swarm/src/connection/pool.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
@umgefahren
Copy link
Contributor Author

While we are on it: In the interest of #2166 we should add support for wasm bindgen futures.

@thomaseizinger
Copy link
Contributor

While we are on it: In the interest of #2166 we should add support for wasm bindgen futures.

Oh that would be amazing actually!

core/Cargo.toml Outdated Show resolved Hide resolved
swarm/src/connection/pool.rs Show resolved Hide resolved
@umgefahren
Copy link
Contributor Author

Let me know as soon as your fine with the changes made. I will then start the process of replacing all usages of the deprecated API.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

API looks good to me apart from one breaking change :)

swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
@umgefahren
Copy link
Contributor Author

Unless you want me to remove #[derive(Default, Debug, Clone, Copy, Hash, PartialEq, Eq, PartialOrd, Ord)] I think there is nothing left to do for me here.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Great work. Happy for this footgun to soon be gone.

@umgefahren could you please also create a pull request removing the deprecated methods? Once this is released we can then merge that second pull request.

swarm/src/connection/pool.rs Outdated Show resolved Hide resolved
swarm/src/connection/pool.rs Outdated Show resolved Hide resolved
swarm/src/connection/pool.rs Show resolved Hide resolved
swarm/src/executor.rs Outdated Show resolved Hide resolved
swarm/src/executor.rs Outdated Show resolved Hide resolved
swarm/src/executor.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Nov 14, 2022

This pull request has merge conflicts. Could you please resolve them @umgefahren? 🙏

@umgefahren
Copy link
Contributor Author

This pull request has merge conflicts. Could you please resolve them @umgefahren? 🙏

That's probably the happiest merge conflict I ever had to resolve. Finally QUIC!!

@thomaseizinger thomaseizinger changed the title swarm: Make Swarm executor aware feat(swarm): Make executor for connection tasks explicit Nov 14, 2022
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Follow up on inline discussion. Otherwise looks good to me.

swarm/src/connection/pool.rs Outdated Show resolved Hide resolved
- Feature-gate `NetworkBehaviour` macro behind `macros` feature flag. See [PR 3055].

- Make executor in Swarm constructor explicit. See [PR 3097].
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the great changelog entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks :)

@mergify mergify bot merged commit d5ea93d into libp2p:master Nov 15, 2022
@mxinden
Copy link
Member

mxinden commented Nov 15, 2022

Again, thanks @umgefahren for the help!

@mxinden
Copy link
Member

mxinden commented Nov 15, 2022

(I am sure many future users not falling into this trap will be very happy (unknowingly).)


#[cfg(feature = "tokio")]
#[derive(Default, Debug, Clone, Copy)]
pub(crate) struct TokioExecutor;
Copy link
Member

Choose a reason for hiding this comment

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

Why not expose TokioExecutor and AsyncStdExecutor? Folks that want to specify configs, e.g. dial_concurrency_factor need to use the SwarmBuilder. Instead of implementing Executor on their custom type wrapping e.g. the async-std executor, they could just reuse our implementation. Thoughts @umgefahren and @thomaseizinger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think you are right. If Thomas agrees I can open a PR real quick changing that and adding some documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we could add the same set of with_ APIs.

I am okay either way. (I thought we had two layers of with_ APIs)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just get rid of SwarmBuilder instead maybe? Is there a reason why any of the config values could not be set on Swarm itself?

Copy link
Member

Choose a reason for hiding this comment

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

Trying to upgrade iroh to laster master and blocked on this :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

I nedd the SwarmBuilder and have to reimplement the Executor trait by copy pasting the one in here, to use tokio.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just get rid of SwarmBuilder instead maybe? Is there a reason why any of the config values could not be set on Swarm itself?

@umgefahren Would you mind:

  • sending a hotfix PR that makes these types public
  • attempting to remove SwarmBuilder altogether by moving the current "builder" functions directly onto Swarm

LMK if you are not keen for the latter, then I'll try myself :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can do that (although I'm done for today)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done #3155

mergify bot pushed a commit that referenced this pull request Nov 23, 2022
mergify bot pushed a commit that referenced this pull request Jan 4, 2023
With #3097, subscribing to the floodsub topic in `examples/chat-tokio.rs` was removed. I assume this was unintentional (cc @umgefahren), so this PR adds it back.
umgefahren added a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
Previously, the executor for connection tasks silently defaulted to a `futures::executor::ThreadPool`. This causes issues such as libp2p#2230.

With this patch, we force the user to choose, which executor they want to run the connection tasks on which results in overall simpler API with less footguns.

Closes libp2p#3068.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

swarm: Make API for creating a new Swarm executor aware
4 participants