Fix _cannot recursively call into Core_ issue

[ngotchac]

It seems that on latest master, Parity would sometimes crash with
Thread 'IO Worker #2' panicked at 'cannot recursively call into "Core"', libcore/option.rs:1008.

This is because we use https://github.com/sbstp/rust-igd method search_gateway_from_timeout which calls get_control_url which runs a future in a new tokio Core, while we call this method already from a running tokio Core.

https://github.com/sbstp/rust-igd/blob/df53f419eec3684eec4bf0f45003cff1c4933682/src/search.rs#L101-L105

Moving to this fork and branch https://github.com/maufl/rust-igd/tree/update-hyper solves the issue ; maybe we should have our own fork?

Instead of using another fork, I found that the easiest way to fix this is actually to run igd::search_gateway_from_timeout in a separate thread, where we are sure tokio won't be already running.

To test that this work, one can apply this diff patch:

diff --git a/util/io/src/worker.rs b/util/io/src/worker.rs
index 458882a85..cab55f8b5 100644
--- a/util/io/src/worker.rs
+++ b/util/io/src/worker.rs
@@ -72,7 +72,9 @@ impl Worker {
 			move || {
 				LOCAL_STACK_SIZE.with(|val| val.set(STACK_SIZE));
 				let ini = (stealer, channel.clone(), wait, wait_mutex.clone(), deleting);
+				let first_call = ::std::sync::atomic::AtomicBool::new(true);
 				let future = future::loop_fn(ini, |(stealer, channel, wait, wait_mutex, deleting)| {
+					if !first_call.swap(false, AtomicOrdering::Relaxed)
 					{
 						let mut lock = wait_mutex.lock();
 						if deleting.load(AtomicOrdering::Acquire) {

The inherant issue is that future::loop_fn first calls the closure, so if the message to run init_public_interface (which calls igd::search_gateway_from_timeout) is already in the queue when setting up the workers, it would run just fine since tokio::runtime::current_thread::block_on_all hasn't been called yet ; this happens in most cases since it's the first message sent to the IoService. It could happen that it isn't, resulting in init_public_interface being called in a Tokio context, crashing parity-ethereum.

With this patch, the first call to the future's closure won't wait for a message, resulting in every message being called in a Tokio context. So if one applies this patch on master, it should crash the app at every single run.

[ngotchac]

The bug first appeared after #9979, so I'm not sure to which versions it applies. (@5chdn)

[niklasad1]

why not

match search_gateway_child.join().ok()? {
   Err(e) => {}
   Ok(g) => {}
}

[niklasad1]

@ngotchac I have seen this when I enable a lot of logs but I don't understand how the logs apply to this? Or do you think it is something else?

[ngotchac]

@niklasad1 I think it's just a race-condition, so enabling logs might just slow down some parts of the start-up and raise this issue.

[niklasad1]

LGTM,

Can you remove the first commit which is confusing IMHO because it was removed in the in the second commit without any explanation?

[sorpaas]

LGTM. We only call it at startup so starting a thread shouldn't be bad.