Skip to content

Remove some Unpin requirements on Futures#1384

Merged
tomaka merged 15 commits into
libp2p:masterfrom
tomaka:rm-unpin
Jan 14, 2020
Merged

Remove some Unpin requirements on Futures#1384
tomaka merged 15 commits into
libp2p:masterfrom
tomaka:rm-unpin

Conversation

@tomaka

@tomaka tomaka commented Jan 9, 2020

Copy link
Copy Markdown
Member

Opening as a draft.
The goal is to remove all Unpin requirements on futures on traits such as *Upgrade or Transport.
All the T::Dial: Unpin/T::Listener: Unpin/T::ListenerUpgrade: Unpin requirements still remain to be cleaned up.

This is surprisingly hard and my progress has stalled when chaining multiple futures is involved.
I've used unsafe code for transport::and_then, but that should ideally be avoided unless no other solution is found.

If someone wants to take over this work, let me know.

@tomaka tomaka marked this pull request as ready for review January 10, 2020 14:36
@tomaka

tomaka commented Jan 10, 2020

Copy link
Copy Markdown
Member Author

This is ready for review!

I ended up cheating a bit by using Pin<Box<...>> where there was no other solution. All the unsafe code has been removed.

There's a breaking change: since the Transport::Listener no longer necessarily implements Unpin, I had to modify remove_listener to not return the Listener stream anymore.

@tomaka

tomaka commented Jan 10, 2020

Copy link
Copy Markdown
Member Author

Note that I removed all the Unpin requirements on futures and streams related to Transports and upgrades, but I haven't touched the ones related to AsyncRead/AsyncWrite. There are around 180 occurrences of Unpin related to AsyncRead/AsyncWrite to remove, so that's for later.

@twittner

Copy link
Copy Markdown
Contributor

The goal is to remove all Unpin requirements on futures on traits such as *Upgrade or Transport. [...] There are around 180 occurrences of Unpin related to AsyncRead/AsyncWrite to remove, so that's for later.

Do you have upgrade, transport or i/o types in mind which must not be moved and where our Unpin bounds present a problem?

@tomaka

tomaka commented Jan 13, 2020

Copy link
Copy Markdown
Member Author

Do you have upgrade, transport or i/o types in mind which must not be moved and where our Unpin bounds present a problem?

Anything that uses async/await does not implement Unpin. #1383 is a concrete example.

@tomaka tomaka merged commit 3f968cb into libp2p:master Jan 14, 2020
@tomaka tomaka deleted the rm-unpin branch January 14, 2020 11:03
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.

2 participants