[Tarantool-patches] [PATCH 1/1] raft: fix crash in worker fiber

[Tarantool-patches] [PATCH 1/1] raft: fix crash in worker fiber

[Vladislav Shpilevoy]

Raft worker fiber does all the heavy and yielding jobs. These are
2 - disk write, and network broadcast. Disk write yields. Network
broadcast is slow, so it happens at most once per event loop
iteration.

The worker on each iteration check if any of these 2 jobs is
active, and if not, it goes to sleep until an explicit wakeup.

But there was a bug. Before going to sleep it did a yield + a
check that there is nothing to do. However during the yield new
tasks could appear, and the check failed, leading to a crash.

The patch reorganizes this part of the code so now the worker does
not yield between checking new tasks and going to sleep.

No test, because extremely hard to reproduce, and don't want to
clog this part of the code with error injections.
---
Branch: http://github.com/tarantool/tarantool/tree/gerold103/raft-crash

 src/box/raft.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/box/raft.c b/src/box/raft.c
index e1e60ce94..914b0d68f 100644
--- a/src/box/raft.c
+++ b/src/box/raft.c
@@ -682,11 +682,11 @@ raft_worker_f(va_list args)
 			raft_worker_handle_broadcast();
 			is_idle = false;
 		}
+		if (is_idle) {
+			assert(raft_is_fully_on_disk());
+			fiber_yield();
+		}
 		fiber_sleep(0);
-		if (!is_idle)
-			continue;
-		assert(raft_is_fully_on_disk());
-		fiber_yield();
 	}
 	return 0;
 }
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/1] raft: fix crash in worker fiber

[Vladislav Shpilevoy]

> diff --git a/src/box/raft.c b/src/box/raft.c
> index e1e60ce94..914b0d68f 100644
> --- a/src/box/raft.c
> +++ b/src/box/raft.c
> @@ -682,11 +682,11 @@ raft_worker_f(va_list args)
>  			raft_worker_handle_broadcast();
>  			is_idle = false;
>  		}
> +		if (is_idle) {
> +			assert(raft_is_fully_on_disk());
> +			fiber_yield();
> +		}
>  		fiber_sleep(0);
> -		if (!is_idle)
> -			continue;
> -		assert(raft_is_fully_on_disk());
> -		fiber_yield();
>  	}
>  	return 0;

I changed this part just a little. Because it makes no sense to
sleep(0) right after a yield. Force pushed to the branch.

====================
@@ -685,8 +685,9 @@ raft_worker_f(va_list args)
 		if (is_idle) {
 			assert(raft_is_fully_on_disk());
 			fiber_yield();
+		} else {
+			fiber_sleep(0);
 		}
-		fiber_sleep(0);
 	}
 	return 0;
 }

Re: [Tarantool-patches] [PATCH 1/1] raft: fix crash in worker fiber

[Serge Petrenko]

08.11.2020 21:04, Vladislav Shpilevoy пишет:
>> diff --git a/src/box/raft.c b/src/box/raft.c
>> index e1e60ce94..914b0d68f 100644
>> --- a/src/box/raft.c
>> +++ b/src/box/raft.c
>> @@ -682,11 +682,11 @@ raft_worker_f(va_list args)
>>   			raft_worker_handle_broadcast();
>>   			is_idle = false;
>>   		}
>> +		if (is_idle) {
>> +			assert(raft_is_fully_on_disk());
>> +			fiber_yield();
>> +		}
>>   		fiber_sleep(0);
>> -		if (!is_idle)
>> -			continue;
>> -		assert(raft_is_fully_on_disk());
>> -		fiber_yield();
>>   	}
>>   	return 0;
> I changed this part just a little. Because it makes no sense to
> sleep(0) right after a yield. Force pushed to the branch.
>
> ====================
> @@ -685,8 +685,9 @@ raft_worker_f(va_list args)
>   		if (is_idle) {
>   			assert(raft_is_fully_on_disk());
>   			fiber_yield();
> +		} else {
> +			fiber_sleep(0);
>   		}
> -		fiber_sleep(0);
>   	}
>   	return 0;
>   }
Hi! Thanks for the patch!
LGTM.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 1/1] raft: fix crash in worker fiber

[Alexander V. Tikhonov]

Hi Vlad, I've checked all results in gitlab-ci, and no new degradations
found [1], patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/213343120

On Sun, Nov 08, 2020 at 07:04:09PM +0100, Vladislav Shpilevoy wrote:
> > diff --git a/src/box/raft.c b/src/box/raft.c
> > index e1e60ce94..914b0d68f 100644
> > --- a/src/box/raft.c
> > +++ b/src/box/raft.c
> > @@ -682,11 +682,11 @@ raft_worker_f(va_list args)
> >  			raft_worker_handle_broadcast();
> >  			is_idle = false;
> >  		}
> > +		if (is_idle) {
> > +			assert(raft_is_fully_on_disk());
> > +			fiber_yield();
> > +		}
> >  		fiber_sleep(0);
> > -		if (!is_idle)
> > -			continue;
> > -		assert(raft_is_fully_on_disk());
> > -		fiber_yield();
> >  	}
> >  	return 0;
> 
> I changed this part just a little. Because it makes no sense to
> sleep(0) right after a yield. Force pushed to the branch.
> 
> ====================
> @@ -685,8 +685,9 @@ raft_worker_f(va_list args)
>  		if (is_idle) {
>  			assert(raft_is_fully_on_disk());
>  			fiber_yield();
> +		} else {
> +			fiber_sleep(0);
>  		}
> -		fiber_sleep(0);
>  	}
>  	return 0;
>  }

Re: [Tarantool-patches] [PATCH 1/1] raft: fix crash in worker fiber

[Vladislav Shpilevoy]

Pushed to master and 2.6.