fix(client): setup separate SyncArbiter for ViewClientActor with 4 threads #2970

mikhailOK · 2020-07-10T06:34:34Z

Change from #2752 + allow storage failures in code reachable from view client.

Test plan

Run existing tests

gitpod-io · 2020-07-10T06:34:38Z

frol · 2020-07-10T07:03:25Z

test-utils/testlib/src/actix_utils.rs

@@ -32,6 +34,8 @@ impl ShutdownableThread {
 impl Drop for ShutdownableThread {
    fn drop(&mut self) {
        self.shutdown();
+        // Leaving some time for all threads to stop after system is stopped.
+        thread::sleep(Duration::from_millis(100));


It seems that it is not a solution, but a symptomatic patch. Is there an issue about this?

I agree, we should be explicitly waiting for all threads to stop.

tests/test_tps_regression.rs

neard/src/main.rs

…reads Change from 2752 + allow storage failures in code reachable from view client. Test plan --------- Run existing tests

Co-authored-by: Vlad Frolov <[email protected]>

neard/tests/rpc_nodes.rs

This reverts commit 2c2ad3f.

bowenwang1996 · 2020-07-10T15:13:51Z

chain/client/src/view_client.rs

+) -> Addr<ViewClientActor> {
+    let request_manager = Arc::new(RwLock::new(ViewClientRequestManager::new()));
+    SyncArbiter::start(config.view_client_threads, move || {
+        // ViewClientActor::start_in_arbiter(&Arbiter::current(), move |_ctx| {


should this line be removed?

bowenwang1996 · 2020-07-10T15:17:43Z

neard/src/lib.rs

@@ -171,5 +166,5 @@ pub fn start_with_config(

    trace!(target: "diagnostic", key="log", "Starting NEAR node with diagnostic activated");

-    (client_actor, view_client)
+    (client_actor, view_client, vec![client_arbiter, arbiter])


why do we need to keep the arbiters?

we join them at the end, it seems like System::run blocks until stop signal and we want to wait for all threads to end, but not 100% sure it's needed

MaksymZavershynskyi

Does it also fix #2948 ?

mikhailOK · 2020-07-10T18:35:50Z

does not fix, only makes it affect chain store

MaksymZavershynskyi

SyncArbiter seems to be the right thing to use here, but does it mean that every other actor in our node runs on system arbiter, including the network and the RPC server? This would mean we are using a single OS thread for everything, but view clients. CC @pmnoxx

I'll delegate reviewing concurrency in this code to @pmnoxx and @frol

Also, @damons mentioned that our storage might allow dirty reads (we are using batched operations, instead of transactions API in rocksdb) so this PR might expose this issue now. If he is right, prepare for nodes returning garbage through RPC. CC @ailisp .

mikhailOK · 2020-07-10T19:27:26Z

Network spawns many threads since #2772
Regular client runs in one thread until we make the bigger storage change
JsonRpc requests that go to view client can now fail with these random storage errors, but we don't know yet how common they will be in practice

MaksymZavershynskyi · 2020-07-10T20:12:56Z

Our JSONRpc server (not to be confused with ViewClienActor) still uses system arbiter, is it correct? If it is so then it means we have one thread for all RPC and regular ClientActor. CC @frol

JsonRpc requests that go to view client can now fail with these random storage errors, but we don't know yet how common they will be in practice

This is quite dangerous. Can we make sure our ViewClientActor does not serve torn data? If it is the case then lots of tools built on top of it will start failing, e.g. the bridge.

SkidanovAlex · 2020-07-10T20:28:48Z

This is quite dangerous. Can we make sure our ViewClientActor does not serve torn data? If it is the case then lots of tools built on top of it will start failing, e.g. the bridge.

Our database is generally set up in a way that we do not modify stuff, we only insert and delete (except for very few exceptions). Correspondingly, the ViewClient will either serve your request successfully (all the data it expected was present throughout), or will fail.

Mostly failures will be due to concurrent GC, in which case the same request sent in ~1 second will also fail anyway.

The alternatives to this fix are:

Implement proper snapshot isolation level. Mikhail is working on it, but will not be done on the timeframe for Phase 1
Keep ViewClientActor with ClientActor on the same thread. That makes it very easy to practically stall block production, even if we disable JsonRPC (via e.g. state sync requests or transactions propagation).

Let's observe how ViewClient actually behaves with this change. In expectation, we will not observe any transient errors.

MaksymZavershynskyi · 2020-07-10T20:45:52Z

Correspondingly, the ViewClient will either serve your request successfully (all the data it expected was present throughout), or will fail.

Since we have multiple columns in the database can it be possible that while block production or block import is happening and we have populated only some of the columns the ViewClient will swoop in and read fresh data from some columns and old data from other columns?

Let's observe how ViewClient actually behaves with this change. In expectation, we will not observe any transient errors.

I would prefer if we were certain before merging in. I am launching a persistent bridge today, and some of our partners will be using it. It would not look nice on us if it breaks because one of our RPC returns garbage data occasionally.

SkidanovAlex · 2020-07-10T21:07:24Z

Since we have multiple columns in the database can it be possible that while block production or block import is happening and we have populated only some of the columns the ViewClient will swoop in and read fresh data from some columns and old data from other columns?

@mikhailOK can correct me if I'm wrong, but I believe that all the affected end points in the ViewClient have the following pattern:

Read A from the storage (say a block). If it doesn't exist, error out
Read B from the storage (say a chunk) that we know exists given A exists. Before this change: panic if failed to read. After this change: trace and error out if failed to read.

This "read B" part can fail for two reasons:

a) We GCed B after A was read. Note that GC would not GC B before it GCed A, but naturally the timing could have been:

VC reads A -> GC deletes A -> GC deleted B -> VC reads B.

In this case the ViewClient will fail with this change (and would not fail before this change because VC and GC are were in the same thread), but it is OK, since A is GCed, such a request is expected to start failing.

b) We inserted A before ViewClient read it, but haven't had a chance to insert B. Indeed, if the block production was not writing atomically, it would be possible:

BP writes A -> VC reads A -> VC attempts to read B -> BP writes B

I see you above mention that we do not use Transaction API, and have a concern that it might imply a possibility for dirty reads. Fortunately, in RocksDB batched writes are atomic, so dirty reads are impossible (second paragraph here).

Note that even with atomic writes, it is still possible to have:

VC reads A -> (atomically BP writes A, BP writes B)

But it's OK, because VC will fail on reading A, and will not attempt to read B, so this data race is also acceptable.

pmnoxx · 2020-07-11T14:17:30Z

SyncArbiter seems to be the right thing to use here, but does it mean that every other actor in our node runs on system arbiter, including the network and the RPC server? This would mean we are using a single OS thread for everything, but view clients. CC @pmnoxx

I'll delegate reviewing concurrency in this code to @pmnoxx and @frol

Also, @damons mentioned that our storage might allow dirty reads (we are using batched operations, instead of transactions API in rocksdb) so this PR might expose this issue now. If he is right, prepare for nodes returning garbage through RPC. CC @ailisp .

Http server starts it's own thread for accepting connections and workers.
PeerManagerActor start in arbiters, which uses it's own thread, and each Peer also has it's own dedicated Arbiter.
As far as I see, everything else runs on the main thread.

There is a bug in Actix library. It looks like Actix doesn't guarantee fairness. While there are messages in mailbox, Actix will prioritize processing messages until mailbox is empty. Execution of any other actors or tasks scheduled with run_later will be delayed.

bowenwang1996 · 2020-07-11T16:04:32Z

@mikhailOK is there something that stops us from merging this PR?

pmnoxx

lgtm

MaksymZavershynskyi · 2020-07-12T23:00:37Z

Thanks @pmnoxx . Since @pmnoxx and @frol approved it, you don't need my approval.

MaksymZavershynskyi · 2020-07-13T16:20:56Z

Actix unfairness sounds like a WAI. AFAIR run_later is designed to not give guarantees on when it is executed, it only guarantees that it is executed after the given time interval, this is from the documentation:

Execute closure after specified period of time within same Actor and Context

Maksym Zavershynskyi NEAR Protocol https://near.org/

…

On Jul 11, 2020, at 4:57 PM, Alexander Skidanov ***@***.***> wrote: Merged #2970 <#2970> into master. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2970 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AILKVB7G52VRCXP37PEOYBLR3D35BANCNFSM4OWJGHJQ>.

We currently call `genesis_state` instead `Chain::new`, which means every time we initialize client or view client we will compute genesis state again. Since #2970 we start 4 view client actors and one client actor, so it means that we could be calling `gensis_state` five times at the same time, which consumes too much memory and doesn't make any sense. This PR fixes it by computing genesis state only once on initialization. Test plan --------- Manually verify that with this fix, we can start a testnet node without needing an absurd amount of memory.

mikhailOK requested review from ilblackdragon, SkidanovAlex and bowenwang1996 July 10, 2020 06:34

mikhailOK requested review from evgenykuzyakov, frol and MaksymZavershynskyi as code owners July 10, 2020 06:34

mikhailOK force-pushed the actix_threads branch 2 times, most recently from c527f90 to e08a5ee Compare July 10, 2020 06:54

frol approved these changes Jul 10, 2020

View reviewed changes

fix(client): setup separate SyncArbiter for ViewClientActor with 4 th…

aaa8e74

…reads Change from 2752 + allow storage failures in code reachable from view client. Test plan --------- Run existing tests

mikhailOK force-pushed the actix_threads branch from e08a5ee to aaa8e74 Compare July 10, 2020 07:13

mikhailOK and others added 2 commits July 10, 2020 00:17

Update neard/src/main.rs

2c2ad3f

Co-authored-by: Vlad Frolov <[email protected]>

revert test_tps_regression

c9f4548

frol reviewed Jul 10, 2020

View reviewed changes

neard/tests/rpc_nodes.rs Outdated Show resolved Hide resolved

Revert "Update neard/src/main.rs"

d8c0637

This reverts commit 2c2ad3f.

mikhailOK linked an issue Jul 10, 2020 that may be closed by this pull request

Split VlewClient and Client into separate threads #2751

Closed

testing

bc3e001

mikhailOK force-pushed the actix_threads branch from b1af8c2 to bc3e001 Compare July 10, 2020 08:18

bowenwang1996 reviewed Jul 10, 2020

View reviewed changes

SkidanovAlex approved these changes Jul 10, 2020

View reviewed changes

MaksymZavershynskyi reviewed Jul 10, 2020

View reviewed changes

Merge branch 'master' into actix_threads

23dbe93

MaksymZavershynskyi reviewed Jul 10, 2020

View reviewed changes

MaksymZavershynskyi requested a review from pmnoxx July 10, 2020 18:42

Merge branch 'master' into actix_threads

626f1b6

bowenwang1996 approved these changes Jul 10, 2020

View reviewed changes

fix view client Chain::new

09126cb

pmnoxx approved these changes Jul 11, 2020

View reviewed changes

SkidanovAlex merged commit a14446f into master Jul 11, 2020

SkidanovAlex deleted the actix_threads branch July 11, 2020 23:58

frol mentioned this pull request Jul 13, 2020

RPC response time is ridiculously long #2984

Closed

weekly-digest bot mentioned this pull request Jul 17, 2020

Weekly Digest (10 July, 2020 - 17 July, 2020) #3003

Closed

bowenwang1996 mentioned this pull request Jul 24, 2020

fix: prevent repetitive initialization on node start #3035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(client): setup separate SyncArbiter for ViewClientActor with 4 threads #2970

fix(client): setup separate SyncArbiter for ViewClientActor with 4 threads #2970

mikhailOK commented Jul 10, 2020

gitpod-io bot commented Jul 10, 2020 •

edited

Loading

frol Jul 10, 2020

MaksymZavershynskyi Jul 10, 2020 •

edited

Loading

bowenwang1996 Jul 10, 2020

bowenwang1996 Jul 10, 2020

mikhailOK Jul 10, 2020

MaksymZavershynskyi left a comment

mikhailOK commented Jul 10, 2020

MaksymZavershynskyi left a comment •

edited

Loading

mikhailOK commented Jul 10, 2020

MaksymZavershynskyi commented Jul 10, 2020

SkidanovAlex commented Jul 10, 2020

MaksymZavershynskyi commented Jul 10, 2020

SkidanovAlex commented Jul 10, 2020 •

edited

Loading

pmnoxx commented Jul 11, 2020

bowenwang1996 commented Jul 11, 2020

pmnoxx left a comment

MaksymZavershynskyi commented Jul 12, 2020

MaksymZavershynskyi commented Jul 13, 2020 via email

fix(client): setup separate SyncArbiter for ViewClientActor with 4 threads #2970

fix(client): setup separate SyncArbiter for ViewClientActor with 4 threads #2970

Conversation

mikhailOK commented Jul 10, 2020

Test plan

gitpod-io bot commented Jul 10, 2020 • edited Loading

frol Jul 10, 2020

Choose a reason for hiding this comment

MaksymZavershynskyi Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

bowenwang1996 Jul 10, 2020

Choose a reason for hiding this comment

bowenwang1996 Jul 10, 2020

Choose a reason for hiding this comment

mikhailOK Jul 10, 2020

Choose a reason for hiding this comment

MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

mikhailOK commented Jul 10, 2020

MaksymZavershynskyi left a comment • edited Loading

Choose a reason for hiding this comment

mikhailOK commented Jul 10, 2020

MaksymZavershynskyi commented Jul 10, 2020

SkidanovAlex commented Jul 10, 2020

MaksymZavershynskyi commented Jul 10, 2020

SkidanovAlex commented Jul 10, 2020 • edited Loading

pmnoxx commented Jul 11, 2020

bowenwang1996 commented Jul 11, 2020

pmnoxx left a comment

Choose a reason for hiding this comment

MaksymZavershynskyi commented Jul 12, 2020

MaksymZavershynskyi commented Jul 13, 2020 via email

gitpod-io bot commented Jul 10, 2020 •

edited

Loading

MaksymZavershynskyi Jul 10, 2020 •

edited

Loading

MaksymZavershynskyi left a comment •

edited

Loading

SkidanovAlex commented Jul 10, 2020 •

edited

Loading