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): keep Connections on the same Task #2536

Closed

Conversation

dignifiedquire
Copy link
Member

@dignifiedquire dignifiedquire commented Feb 24, 2022

We now upgrade a PendingConnection on the same Task, instead of moving it back to the
main Task, and spawning another one.

@mxinden this is a first pass at what we talked about, in regards to keeping the connection on a single task once established.

Depends on #2535

We know upgrade a PendingConnection on the same Task, instead of moving it back to the
main Task, and spawning another one
@dignifiedquire dignifiedquire changed the title feat: remove Send bound from NetworkBehaviour feat(swarm): keep Connections on the same Task Feb 24, 2022
@mxinden
Copy link
Member

mxinden commented Feb 24, 2022

Mind splitting the PR in two? 0c5fa28 we can merge soon. 88cc599 I want to give a bit more thought.

@dignifiedquire
Copy link
Member Author

@mxinden #2535

@thomaseizinger
Copy link
Contributor

Thanks @dignifiedquire ! Do you mind adding some rationale on why this is better? :)

Is it supposedly easier to understand? Less susceptible to certain error conditions? Better encapsulation?

@dignifiedquire
Copy link
Member Author

Is it supposedly easier to understand? Less susceptible to certain error conditions? Better encapsulation?

The idea of doing this, arose from me working on trying to integrate rust-libp2p with https://github.com/DataDog/glommio/. With that executor, connections are not necessarily Send, and so I was piece wise figuring out how to avoid moving things between threads.
With this change, in theory, a connection can be guranteed to be kept on a single thread after having been spawned (assuming that is what the executor does of course).

@thomaseizinger
Copy link
Contributor

Thanks for that!

I'll have a look with that in mind later :)

@thomaseizinger
Copy link
Contributor

I think in principle, I am okay with this direction.

The idea of doing this, arose from me working on trying to integrate rust-libp2p with DataDog/glommio. With that executor, connections are not necessarily Send, and so I was piece wise figuring out how to avoid moving things between threads.

Does this mean, only the final connection is !Send but the task for creating the connection is Send? With #2652, we will be emitting individual "upgrade" futures from a Transport (and Transport will no longer be Clone). Consequently, there will be a task that polls the underlying transport but the upgrade will be put into its own task which might be on another thread.

@thomaseizinger thomaseizinger added the need/author-input Needs input from the original author label Nov 2, 2022
@github-actions github-actions bot added the Stale label Jan 2, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 2, 2023

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

@github-actions github-actions bot removed the Stale label Jan 3, 2023
@github-actions github-actions bot added the Stale label Mar 4, 2023
@mergify
Copy link
Contributor

mergify bot commented Mar 4, 2023

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

@github-actions github-actions bot removed the Stale label Mar 5, 2023
@github-actions github-actions bot added the Stale label May 5, 2023
@thomaseizinger
Copy link
Contributor

I guess we can close this as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/author-input Needs input from the original author Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants