Fix light client deadlock

[cheme]

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

[parity-cla-bot]

It looks like @cheme signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[andresilva]

Nice job on finding this. For the Informant case it's ok to just return false from is_major_importing since it will possibly just delay the printing of the import message.

But the underlying issue is still present and could happen again in the future through other listeners. I think we should either have more fine-grained locking on state, or make sure the calls to the various ChainNotify subscribers are handled outside of the state lock (this doesn't seem too easy to do).

I'm not very familiar with this code so it's possible my suggestions don't make much sense. I'm also OK with merging this right now since it's an easy fix.

[andresilva]

I think rustc probably handles this already. I don't have a strong opinion on it though.

[cheme]

Me neither, I am just used to add those (I never really checked). I believe there is not a lot in parity code so I will remove it (it feels reasonable to rely on the compiler :)

[andresilva]

I'm not sure we want to add this method to the trait since it exposes an implementation detail of LightSync. Just adding it to LightSync should be fine since that's also the type we use in Informant.

[cheme]

I did not like to add this method in the trait, but in fact Informant (the light one) uses the trait, seems to be a design choice to only have read_only method accessible to the informant (alas read_only does not mean no deadlock). I will rename the trait method to make it a bit less technical.

[andresilva]

When acquiring the state lock would block we return false.?

[cheme]

I concur with your analysis, the underlying issue is still present, it is not easy to get a global view over all the lock used; especially with listeners and io_handlers (the sync nature of their calls makes it very likely to trigger others deadlock with future change). Ideally we should ensure there is no circular reference, and maybe use asynch queue to avoid some. Issue like #9114 (circular reference) is definitely present in the light client at some places.