Update to stables futures #81
Conversation
| } | ||
|
|
||
| struct Buffered<S: Sink> { | ||
| struct Buffered<S, I> { |
There was a problem hiding this comment.
Note: the signature of the Sink trait has changed. The item is now a parameter of the trait instead of an associated type.
Also note that there is now a Buffered type provided by the futures-preview crate, but I'll open an issue for that.
There was a problem hiding this comment.
Nice - it didn't exist when I wrote this code.
|
|
||
| ::futures::future::join_all(finalized_streams).map(|_| signal.fire()) | ||
| })).unwrap(); | ||
| threads_pool.spawn_ok(routing_task.map(|_| ())); |
There was a problem hiding this comment.
Note: Because the behaviour of join_all has changed a bit, I had to adjust the order in which things are initialized, otherwise the test wouldn't work (blocked at round 1 with futures not being notified). This is a bit concerning, but I couldn't find the origin of the issue.
There was a problem hiding this comment.
From what I can tell this is not related to join_all. If the routing_task future is spawned before the first Voter is created (i.e. the case where the test stalls / hangs), the NetworkRouting future is polled "immediately" before the first voter is created. It then returns Poll::Pending, which it always does, since it relies on task wakeup from the UnboundedReceivers (i.e. when messages are sent). However, there are no voters and hence no VotingRound yet, so the current task is only scheduled to be polled again if a message is sent to the GlobalMessageNetwork, which doesn't happen in this test. So the NetworkRouting is never polled again, even after the first round with voters is created, resulting in the messages never being dispatched.
In all other tests except one (skips_to_latest_round_after_catch_up), the routing_task is only spawned after the first voter is created. In skips_to_latest_round_after_catch_up it is spawned earlier, but this test then calls network.send_message after a voter is set up, i.e. sends a message on the GlobalMessageNetwork, resulting in the NetworkRouting to be polled again.
The reason this worked previously is most likely due to the semantics of block_on_all in tokio, which results in the spawned "inner" NetworkRouting future to be polled again after the "outer" future resolved, at which point the voters have been set up and a voting round has been created.
So it seems to me that spawning the routing_task only after at least one voter has been created is not only necessary now but arguably more correct, at least the way the NetworkRouting is currently set up.
There was a problem hiding this comment.
Thanks for looking into this and for the detailed explanation @romanb.
Codecov Report
@@ Coverage Diff @@
## master #81 +/- ##
==========================================
- Coverage 90.79% 88.26% -2.54%
==========================================
Files 10 10
Lines 3390 2480 -910
==========================================
- Hits 3078 2189 -889
+ Misses 312 291 -21
Continue to review full report at Codecov.
|
|
Ping! |
andresilva
left a comment
andresilva
left a comment
There was a problem hiding this comment.
lgtm! only thing a bit worrying is that issue with join_all in the test.
|
@tomaka sorry for being a shitty maintainer 😿 |
|
Yup I also gave this another look after the ping, also worried by the |
|
See this comment regarding the mentioned concern. |
|
Should we merge this? |
5 participants
Updates the crate to use futures from the standard library rather than the
futurescrate.