Switch to stable futures #1196

tomaka · 2019-07-09T13:04:03Z

I'm opening this PR to show that I've started working on it, but it's nowhere close to being ready.
Considering that this is will force project which depend on rust-libp2p to upgrade to stable futures as well, we should maybe do a separate release just with this PR.

We're depending on futures-preview 0.3.0-alpha.17. It's not great to depend on such a version, because they aren't compatible with any other.

For the Future, Context and Poll items it's not a problem, because the library re-exports these symbols from the Rust standard library. Therefore if a function returns a Poll coming from futures-preview 0.3.0-alpha.17, it is the same Poll as the one in futures-preview 0.3.0-alpha.16 for example.

For Sink, Stream, AsyncRead and AsyncWrite, this is more problematic, and I hope that a version 0.3.0 of futures-preview will be eventually released.

cc #993

tomaka · 2019-09-11T15:22:09Z

If nobody is against this, let's merge this branch into the stable-futures branch, despite the fact that it doesn't compile.
Then we will continue opening PRs against it to finish the changes.

mxinden

Just a basic pass. Comments can either be addressed here or with a follow up pull request. Both is fine by me.

I would suggest merging this branch (tomaka:new-futures) into libp2p/stable-futures. Once that happened all follow up new-future related changes can be merged into libp2p/stable-futures via pull requests, either being reviewed right away, or reviewed as a final pass over the entire libp2p/stable-futures branch before merging it into master.

mxinden · 2019-09-12T08:47:07Z

core/src/nodes/handled_node/from_fn.rs

+
+#![cfg(feature = "async-await")]
+
+pub struct FromFn<THandler> {


Is this file in work-in-progress state? Can you add some doc comments to understand the purpose of the file?

In case this is clear for anyone familiar with the codebase, please disregard my comment.

mxinden · 2019-09-12T08:50:32Z

core/src/nodes/listeners.rs

-    }
-
-    #[test]
-    fn listener_stream_returns_transport() {


Are all of these tests obsolete?

mxinden · 2019-09-12T08:56:06Z

core/src/nodes/tasks/manager.rs

-        self.to_spawn.push(task);
+        let task = Box::pin(Task::new(task_id, self.events_tx.clone(), rx, future, handler));
+        if let Some(threads_pool) = &mut self.threads_pool {
+            threads_pool.spawn(task).expect("spawning a task on a threads pool never fails; qed");


Suggested change

threads_pool.spawn(task).expect("spawning a task on a threads pool never fails; qed");

threads_pool.spawn(task).expect("spawning a task on a thread pool never fails; qed");

mxinden · 2019-09-12T08:59:10Z

core/src/nodes/tasks/manager.rs


-        let task: Task<futures::future::Empty<_, _>, _, _, _, _, _, _> =
+        // TODO: we use `Pin<Box<Pending>>` instead of just `Pending` because `Pending` doesn't


Your patch should be included in futures-rs 0.3.0-alpha.18, right? What is stopping us from updating to 0.3.0-alpha.18?

mxinden · 2019-09-12T09:07:01Z

core/src/transport/timeout.rs

@@ -136,40 +143,44 @@ where
 // TODO: can be replaced with `impl Future` once `impl Trait` are fully stable in Rust
 //       (https://github.com/rust-lang/rust/issues/34511)
 #[must_use = "futures do nothing unless polled"]
-pub struct TokioTimerMapErr<InnerFut> {
+pub struct Timeout<InnerFut> {


Why are we not using something like https://github.com/rustasync/futures-timer here?

We are. The provided Timeout wrapper doesn't fit what we want though.

mxinden · 2019-09-12T09:19:04Z

muxers/mplex/Cargo.toml

-tokio-codec = "0.1"
-tokio-io = "0.1"
-unsigned-varint = { version = "0.2.1", features = ["codec"] }
+unsigned-varint = { git = "https://github.com/tomaka/unsigned-varint", branch = "futures-codec", features = ["codec"] }


Can you add a TODO here so we make sure we use either *unsigned-varint` with futures-codec, or unsigned-varint with an updated tokio-codec?

mxinden · 2019-09-12T09:20:02Z

muxers/mplex/src/lib.rs

-/// `Ready(Some())` is almost always returned. An error is returned if the stream is EOF.
-fn next_match<C, F, O>(inner: &mut MultiplexInner<C>, mut filter: F) -> Poll<O, IoError>
-where C: AsyncRead + AsyncWrite,
+/// If `Pending` is returned, the waker is kept and notifier later, just like with any `Poll`.


Suggested change

/// If `Pending` is returned, the waker is kept and notifier later, just like with any `Poll`.

/// If `Pending` is returned, the waker is kept and notified later, just like with any `Poll`.

mxinden · 2019-09-12T09:23:39Z

protocols/kad/Cargo.toml

@@ -28,7 +28,7 @@ tokio-codec = "0.1"
 tokio-io = "0.1"
 wasm-timer = "0.1"
 uint = "0.8"
-unsigned-varint = { version = "0.2.1", features = ["codec"] }
+unsigned-varint = { git = "https://github.com/tomaka/unsigned-varint", branch = "futures-codec", features = ["codec"] }


Same as above. Can you add a TODO so we make sure to clean this up?

mxinden · 2019-09-12T09:25:44Z

protocols/secio/Cargo.toml

 sha2 = "0.8.0"
 hmac = "0.7.0"
+unsigned-varint = { git = "https://github.com/tomaka/unsigned-varint", branch = "futures-codec", features = ["codec"] }


Same as above. Can you add a TODO so we don't forget to clean this up?

mxinden · 2019-09-12T09:28:41Z

swarm/src/lib.rs

+    type Item = Result<TBehaviour::OutEvent, io::Error>;
+
+    fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll<Option<Self::Item>> {
+        // We use a `this` variable to solve borrowing issues.


Suggested change

// We use a `this` variable to solve borrowing issues.

// We use a `this` variable because the compiler can't mutably borrow multiple times

// accross a `Deref`.

Let's keep these comments the same across the codebase so we can do an easy find-replace in case this can ever be solved in a prettier way.

tomaka · 2019-09-16T09:08:29Z

I'll address the review in a follow-up PR.

* Switch to stable futures * Remove from_fn * Fix secio * Fix core --lib tests

Signal the end of stream as `None` instead of producing a broken pipe error. This restores the behaviour prior to PR libp2p#1196. I could not find a motivation for this particular change in the PR and the previous implementation looks correct to me.

Signal the end of stream as `None` instead of producing a broken pipe error. This restores the behaviour prior to PR #1196. I could not find a motivation for this particular change in the PR and the previous implementation looks correct to me.

tomaka force-pushed the new-futures branch 3 times, most recently from ff0aa9e to ba4e2f7 Compare July 9, 2019 15:58

tomaka mentioned this pull request Jul 9, 2019

Questions on futures/async/await roadmap #993

Closed

tomaka force-pushed the new-futures branch 10 times, most recently from fd5f3d6 to 02d00cc Compare July 14, 2019 20:03

tomaka force-pushed the new-futures branch 16 times, most recently from 2cb77ec to c6adf00 Compare July 20, 2019 12:29

tomaka force-pushed the new-futures branch 10 times, most recently from 1d940df to 3c070ca Compare September 4, 2019 14:08

tomaka force-pushed the new-futures branch 3 times, most recently from fe62305 to ba7222e Compare September 11, 2019 09:48

Switch to stable futures

a0e0c32

tomaka force-pushed the new-futures branch from ba7222e to a0e0c32 Compare September 11, 2019 10:05

tomaka changed the base branch from master to stable-futures September 11, 2019 15:20

tomaka marked this pull request as ready for review September 11, 2019 15:20

mxinden reviewed Sep 12, 2019

View reviewed changes

tomaka added 3 commits September 12, 2019 11:46

Remove from_fn

f3e767d

Fix secio

d7ac24f

Fix core --lib tests

e9a33d2

tomaka merged commit 170d2d2 into libp2p:stable-futures Sep 16, 2019

tomaka deleted the new-futures branch September 16, 2019 09:13

tomaka mentioned this pull request Sep 16, 2019

Address some reviewing #1246

Merged

masonforest pushed a commit to masonforest/rust-libp2p that referenced this pull request Jan 7, 2020

Switch to stable futures (libp2p#1196)

8129756

* Switch to stable futures * Remove from_fn * Fix secio * Fix core --lib tests

twittner mentioned this pull request Jul 30, 2020

memory: Do not return broken pipe error on EOF. #1683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to stable futures #1196

Switch to stable futures #1196

tomaka commented Jul 9, 2019 •

edited

tomaka commented Sep 11, 2019

mxinden left a comment

mxinden Sep 12, 2019

mxinden Sep 12, 2019

mxinden Sep 12, 2019

mxinden Sep 12, 2019

mxinden Sep 12, 2019

tomaka Sep 16, 2019

mxinden Sep 12, 2019

mxinden Sep 12, 2019

mxinden Sep 12, 2019

mxinden Sep 12, 2019

mxinden Sep 12, 2019

tomaka commented Sep 16, 2019


		#![cfg(feature = "async-await")]

		pub struct FromFn<THandler> {

	threads_pool.spawn(task).expect("spawning a task on a threads pool never fails; qed");
	threads_pool.spawn(task).expect("spawning a task on a thread pool never fails; qed");


		let task: Task<futures::future::Empty<_, _>, _, _, _, _, _, _> =
		// TODO: we use `Pin<Box<Pending>>` instead of just `Pending` because `Pending` doesn't

	/// If `Pending` is returned, the waker is kept and notifier later, just like with any `Poll`.
	/// If `Pending` is returned, the waker is kept and notified later, just like with any `Poll`.

	// We use a `this` variable to solve borrowing issues.
	// We use a `this` variable because the compiler can't mutably borrow multiple times
	// accross a `Deref`.

Switch to stable futures #1196

Switch to stable futures #1196

Conversation

tomaka commented Jul 9, 2019 • edited

tomaka commented Sep 11, 2019

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka commented Sep 16, 2019

tomaka commented Jul 9, 2019 •

edited