Refactor the crate to use stable futures

[tomaka]

Note: This PR should be merged using the "Merge" action of GitHub, in order to not lose history.

Rewrites lots of parts of the library to use stable futures rather than futures 0.1.
This unlocks the possibility to use async/await, and opens the door for further API refactorings later on.

There is one test failure right now, the "network simultaneous dial" test, which seems to be caused by the V1Lazy upgrade system. This needs to be investigated.

When it comes to reviewing, I don't know what everyone's opinion is. While many individual changes to the stable-futures branch have already been reviewed, unfortunately most of the changes made early on (notably in core and swarm haven't). I think most of the reviewing should be straight-forward, as most of the code is a 1:1 port of the old code.

A few things have changed:

• ProtocolsHandler::poll doesn't return an Error anymore. Instead, ProtocolsHandlerEvent (the return value of poll) has a new Close variant, which corresponds to what an error represented before.
• We're using async-std for networking, rather than tokio. This is for two reasons: this unties us from tokio (async-std futures can be polled with anything, while tokio networking futures can only be polled with tokio), and async-std was ready before tokio was.
• All the traits that have a poll function (i.e. NetworkBehaviour, ProtocolsHandler, NodeHandler) now have a &mut Context parameter to reflect the changes in the Future trait.
• The core::upgrade module was refactored. It now contains a few async methods that allow reading or writing a length-delimited message from a socket.

[twittner]

Another one to do:

test identify::tests::periodic_id_works ... test identify::tests::periodic_id_works has been running for over 60 seconds

So the reason for this is that when the dialling swarm tries to connect to the address before the listener has bound to it, the connection refused error is eventually ignored. It will bubble up through the layers until finally reaching the swarm as a NetworkEvent::UnknownPeerDialError (which in itself is a misclassification as network.rs assumes in handle_reach_error that all other_reach_attempts are inbound). Then the behaviour callback will be invoked (i.e. inject_addr_reach_failure) which by default just ignores the error. Presumably, the NetworkBehaviour impl of Identify could map such errors to its own output event?

[tomaka]

Presumably, the NetworkBehaviour impl of Identify could map such errors to its own output event?

Identify itself never opens any connection and passively operates on existing connections, so I don't think it makes sense to propagate a dialing error here.

[twittner]

Identify itself never opens any connection and passively operates on existing connections, so I don't think it makes sense to propagate a dialing error here.

Makes sense, but I am curious where such errors should be handled?

(On a related note, the Stream::Item of ExtendedSwarm is currently a Result<TBehaviour::OutEvent, io::Error>, but no io::Error is ever produced.)

[Demi-Marie]

Looks good! I have a few comments, but that is all.

[Demi-Marie]

This is a very useful design pattern for Rust futures. Thanks!

[Demi-Marie]

What happens in release mode? It seems wrong to continue in an invalid state.

[mxinden]

We could panic in release mode as well. But given that MDns is probably only a auxiliary component, I think panicing would be a bit exaggerated. In case you have suggestions how we could enforce MaybeBusyMdnsService to be Free at compile time, I am happy to do a follow up pull request.

[Demi-Marie]

Ditto

[Demi-Marie]

Ideally we would do protobuf generation in build.rs. This is okay so long as we are not missing out on upstream security fixes.

[Demi-Marie]

Why this change?

[tomaka]

Because of changes in the Sink trait

[Demi-Marie]

I think we should poll the inner future first.

[Demi-Marie]

Why?

[tomaka]

It's how the WebCrypto API is defined. Also, this is old code not relevant to this PR.

[Demi-Marie]

Should we switch to trust-dns? Not now, obviously.

[romanb]

I skimmed most of it. Kudos to everyone involved in this PR, this really seems to have been a lot of work (and a lot of it chore).

[romanb]

How come this explicit impl is necessary?

[tomaka]

Otherwise Unpin is implemented automatically only if all the fields are Unpin as well.
It is never a bad idea to implement Unpin for a type (provided you don't use unsafe). The reason why we do it for this type is because it implements the Stream trait, but we could theoretically implement Unpin for all the types we define.

[romanb]

What does the use of both 's and 'a with 's: 'a accomplish?

[tomaka]

I think that this is a bug with Rust in general. I will try to make it compile by removing a bound, but the chances are high that it's not possible.

[tomaka]

This:

pub fn send_event<'s>(&'s mut self, event: TInEvent) -> impl Future<Output = ()> + 's {

Results in:

error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds
    --> core/src/nodes/network.rs:1647:61
     |
1647 |     pub fn send_event<'s>(&'s mut self, event: TInEvent) -> impl Future<Output = ()> + 's {

And this:

pub fn send_event(&mut self, event: TInEvent) -> impl Future<Output = ()> + 'a {

Leads to:

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
    --> core/src/nodes/network.rs:1649:34

[romanb]

But why not

    pub fn send_event(&'a mut self, event: TInEvent) -> impl Future<Output = ()> + 'a {

which compiles and expresses what I would expect (that the returned future cannot outlive self which contains references with lifetime 'a).

[romanb]

Suggested change

	/// If no executor is available, we move tasks to this list, and futures are polled on the
	/// If no executor is available, we move tasks to this set, and futures are polled on the

[romanb]

Maybe at least a log statement meanwhile?

[tomaka]

I'll add the logging, but note that this isn't supposed to happen in practice. The user is supposed to call poll_ready_broadcast beforehand, to make sure that room is available. And since both these functions require &mut self, they're not racy.

[twittner]

And since both these functions require &mut self, they're not racy.

I do not think this implication is valid. Given a type T with two operations a and b, each of which requires &mut self, we can wrap T in a Mutex, e.g. Arc<Mutex<T>> and have:

// thread 1 (t1: Arc<Mutex<T>>):
t1.lock().a();
t1.lock().b();

// thread 2 (t2: Arc<Mutex<T>>):
t2.lock().a();
t2.lock().b();

where the MutexGuard is dropped after each call to a or b, resulting in sequences such as a a b b for example.

[tomaka]

That is true for any Rust struct that exists.
The point is that you can use them in a non-racy way. We can't prevent stupidity.

[twittner]

We can't prevent stupidity.

The current API in master apparently can.

[romanb]

Maybe at least log something?

[tomaka]

Same as above. poll_ready_broadcast would return an error if the channel is closed.
This time it's possible for the function to be racy (as the background task can close the channel after poll_ready_broadcast but before start_broadcast), but the API is racy anyway as the background task can be in the process of closing the channel while start_broadcast is being called.

[tomaka]

I don't think we should add any logging, as this is actually a normal path of execution. Network connections can close at any point, and this is what happens if a connection has been closed.

[romanb]

Similar to the other case, I'm wondering why such impls are necessary. Just curious.