Clean up tests from runtime.block_on() and moving around Runtime and Handle
#12941
Conversation
…-block-on-in-tests
dmitry-markin
added
A0-please_review
|
|
||
| let api = Arc::new(two_validators::TestApi {}); | ||
| let beefy_peers = peers.iter().enumerate().map(|(id, key)| (id, key, api.clone())).collect(); | ||
| runtime.spawn(initialize_beefy(&mut net, beefy_peers, min_block_delta)); | ||
| tokio::spawn(initialize_beefy(&mut net, beefy_peers, min_block_delta)); |
There was a problem hiding this comment.
What happens with this task when the test ends, is it also killed because test is marked as #[tokio::test]?
I mean, when multiple #[tokio::test]s are run concurrently does tokio have some magic under the hood to know which tokio::spawn()ed task belongs to which test?
Old version of the code had clear task lifetime -> the lifetime of runtime; with this code, I'm not sure. If you know the answer, please also add a code-comment explanation for others like me 😄
There was a problem hiding this comment.
#[tokio::test] is equivalent to something like Runtime::new().unwrap().block_on(async {...}), so every test gets its own runtime that is dropped at the end. A little more accurate definition is in the docs.
@acatangiu I'm afraid there are too many tokio::spawn() across the PR to add comments (I counted 44 =)
There was a problem hiding this comment.
thank you! tokio::test and tokio::spawn documentation clearly supports what you are saying, sorry for not researching it before asking. It's fine to skip the comments.
| futures::future::poll_fn::<(), _>(|cx| { | ||
| net.poll(cx); | ||
| for peer in 0..3 { | ||
| if net.peer(peer).num_peers() != 2 { | ||
| return Poll::Pending | ||
| } | ||
| } | ||
| Poll::Ready(()) |
There was a problem hiding this comment.
I still find it suspicious that for a simple test like this we need 2 threads.
There was a problem hiding this comment.
I just put flavor = "multi_thread" into every test in sync.rs to not spend too much time on thinking, which tests need it and which doesn't, and also to make the file consistent. In fact, only the following tests hang on a single-threaded runtime:
sync::block_announce_data_is_propagated
sync::multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled
sync::sync_blocks_when_block_announce_validator_says_it_is_new_best
sync::sync_to_tip_when_we_sync_together_with_multiple_peers
sync::syncs_header_only_forks
sync::wait_until_deferred_block_announce_validation_is_ready
The others run fine.
There was a problem hiding this comment.
+1, why do all these sync tests now need multi-threaded runtime? they should work in single thread too right? (also previous code version looks like it was single threaded runtime)
There was a problem hiding this comment.
They should, but they don't due to some nasty blocking/uneven polling/etc. issues. Previously, the runtime created explicitly was multi-threaded (by default), and these issues were not obvious. I tried investigating why, but didn't succeed. As we discussed with @bkchr and @altonen, it doesn't worth putting more time into it, so I just put flavor = "multi_thread" there.
| futures::future::poll_fn::<(), _>(|cx| { | ||
| net.poll(cx); | ||
| for peer in 0..3 { | ||
| if net.peer(peer).num_peers() != 2 { | ||
| return Poll::Pending | ||
| } | ||
| } | ||
| Poll::Ready(()) |
There was a problem hiding this comment.
+1, why do all these sync tests now need multi-threaded runtime? they should work in single thread too right? (also previous code version looks like it was single threaded runtime)
This is a follow-up PR to #12646. It aims to clean up tests from moving around
tokio::runtime::Runtimeandtokio::runtime::Handle, and replace multipleruntime.block_on()with#[tokio::test].