Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Commit

Permalink
Fix light client deadlock (#9385)
Browse files Browse the repository at this point in the history
This PR is fixing deadlock for #8918 

It avoids some recursive calls on light_sync by making state check optional for Informant.

The current behavior is to display the information when informant checks if block is major version.
This change a bit the informant behavior, but not on most cases.

To remember where and how this kind of deadlock are likely to happen (not seen with Parkinglot deadlock detection because it uses std condvar), I am adding a description of the deadlock.
Also, for the reviewers there may be better solution than modifying the informant.

### Thread1 

- ethcore/sync/light_sync/mod.rs

A call to the light handler through any Io (having a loop of rpc query running on like client makes the dead lock way more likely).
At the end of those calls we systematically call `maintain_sync` method.

Here maintain_sync locks `state` (it is the deadlock cause), with a write purpose

`maintain_sync` -> `begin_search` with the state locked open

`begin_search` -> lightcliennt `flush_queue` method

- ethcore/light/src/client/mod.rs

`flush_queue` -> `flush` on queue (HeaderQueue aka VerificationQueue of headers)

- ethcore/src/verification/queue/mod.rs

Condition there is some unverified or verifying content

`flush` wait on a condvar until the queue is empty. The only way to unlock condvar is that worker is empty and unlock it (so thread 2 is Verification worker).

### Thread2

A verification worker at the end of a verify loop (new block).

- ethcore/src/verification/queue/mod.rs

thread loops on `verify` method.

End of loop condition is_ready -> Import the block immediately 

calls `set_sync` on QueueSignal which send a BlockVerified ClientIoMessage in inner channel (IoChannel of ClientIoMessage) using `send_sync`

- util/io/src/service_mio.rs

IoChannel `send_sync` method calls all handlers with `message` method; one of the handlers is ImportBlocks IoHandler (with a single inner Client service field)

- ethcore/light/src/client/service.rs

`message` trigger inner method `import_verified`

- core/light/src/client/mod.rs

`import_verified` at the very end notify the listeners of a new_headers, one of the listeners is Informant `listener` method

- parity/informant.rs

`newHeaders` run up to call to `is_major_importing` on its target (again clinet)

-  ethcore/sync/src/light_sync/mod.rs

Here `is_major_importing` tries to get state lock (read purpose only) but cannot because of previous state lock, thus deadlock
  • Loading branch information
cheme authored and andresilva committed Sep 4, 2018
1 parent c12447c commit c1aed4a
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 19 deletions.
38 changes: 29 additions & 9 deletions ethcore/sync/src/light_sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,27 @@ impl<L: AsLightClient> LightSync<L> {
};
}
}

fn is_major_importing_do_wait(&self, wait: bool) -> bool {
const EMPTY_QUEUE: usize = 3;

if self.client.as_light_client().queue_info().unverified_queue_size > EMPTY_QUEUE {
return true;
}
let mg_state = if wait {
self.state.lock()
} else {
if let Some(mg_state) = self.state.try_lock() {
mg_state
} else {
return false;
}
};
match *mg_state {
SyncState::Idle => false,
_ => true,
}
}
}

// public API
Expand Down Expand Up @@ -645,6 +666,9 @@ pub trait SyncInfo {

/// Whether major sync is underway.
fn is_major_importing(&self) -> bool;

/// Whether major sync is underway, skipping some synchronization.
fn is_major_importing_no_sync(&self) -> bool;
}

impl<L: AsLightClient> SyncInfo for LightSync<L> {
Expand All @@ -657,15 +681,11 @@ impl<L: AsLightClient> SyncInfo for LightSync<L> {
}

fn is_major_importing(&self) -> bool {
const EMPTY_QUEUE: usize = 3;

if self.client.as_light_client().queue_info().unverified_queue_size > EMPTY_QUEUE {
return true;
}
self.is_major_importing_do_wait(true)
}

match *self.state.lock() {
SyncState::Idle => false,
_ => true,
}
fn is_major_importing_no_sync(&self) -> bool {
self.is_major_importing_do_wait(false)
}

}
20 changes: 10 additions & 10 deletions parity/informant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ impl InformantData for LightNodeInformantData {
fn executes_transactions(&self) -> bool { false }

fn is_major_importing(&self) -> bool {
self.sync.is_major_importing()
self.sync.is_major_importing_no_sync()
}

fn report(&self) -> Report {
Expand Down Expand Up @@ -422,15 +422,15 @@ impl LightChainNotify for Informant<LightNodeInformantData> {
if ripe {
if let Some(header) = good.last().and_then(|h| client.block_header(BlockId::Hash(*h))) {
info!(target: "import", "Imported {} {} ({} Mgas){}",
Colour::White.bold().paint(format!("#{}", header.number())),
Colour::White.bold().paint(format!("{}", header.hash())),
Colour::Yellow.bold().paint(format!("{:.2}", header.gas_used().low_u64() as f32 / 1000000f32)),
if good.len() > 1 {
format!(" + another {} header(s)",
Colour::Red.bold().paint(format!("{}", good.len() - 1)))
} else {
String::new()
}
Colour::White.bold().paint(format!("#{}", header.number())),
Colour::White.bold().paint(format!("{}", header.hash())),
Colour::Yellow.bold().paint(format!("{:.2}", header.gas_used().low_u64() as f32 / 1000000f32)),
if good.len() > 1 {
format!(" + another {} header(s)",
Colour::Red.bold().paint(format!("{}", good.len() - 1)))
} else {
String::new()
}
);
*last_import = Instant::now();
}
Expand Down

0 comments on commit c1aed4a

Please sign in to comment.