Re: (ITS#5454) syncrepl refreshAndPersist stops receiving

[rein@basefarm.no]

Howard Chu skrev:
> rein@basefarm.no wrote:

>> Hm, I hope I have found the race condition that causes this :-)  I'm now
>> running with the patch at the end to see if that solves it, only time
>> will tell..

And we haven't seen any replication problems since this was implemented 
friday afternoon :-).  But it is still a bit too early to conclude that 
it has been fixed.

>> The race is that between the time selecting on the syncrepl socket is
>> enabled by the call to connection_client_enable() and the release of the
>> si_mutex a new message may arrive.  If so, the next call to do_syncrepl
>> may fail in its attempt to trylock the mutex and no-one will re-enable
>> selecting on it again.  My patch delays enabling of the socket until the
>> mutex has been released, which looks safe to me.  Or can the access to
>> si->si_conn without a lock be a problem?
> 
> How about just moving the enable to after the runqueue manipulation is
> done?

That would still leave a glitch in the window open, unless the initial 
trylock is changed to a regular lock as your checking message suggests.

But, doing that could result in all the threads ending up waiting for 
the lock if do_syncrepl of a refresh only replication is configured to 
run too often.  I assume the trylock is there to avoid this?  What about 
starting out with deferring the next scheduled call, as a scheduled call 
  is not wanted until the first has completed anyhow?  And a rescedule 
is already being done before do_syncrepl finishes.

Deferring should not be necessary if rtask->next_sched.tv_sec is zero, 
i.e it has already been deferred. That would remove these calls for all 
the cases when do_syncrepl is called as a result of a new message on a 
persistent syncrepl connection.

> Just need to be sure that do_syncrepl() isn't entered again before
> si->si_conn gets initialized.

I'm not quite sure what you mean here.  As I see it, the problem is that 
do_syncrepl() must not be called after si->si_conn is enabled and the 
lock is still held.

> It also occurs to me that we probably don't even need to manipulate the
> slapd runqueue in persist mode, when si->si_conn is already set. I.e.,
> in that case we can only have gotten here because of a listener event,
> and not because of a runqueue schedule.

That is probably true.

Rein