• guybe7's avatar
    Sort out mess around propagation and MULTI/EXEC (#9890) · 7ac21307
    guybe7 authored
    The mess:
    Some parts use alsoPropagate for late propagation, others using an immediate one (propagate()),
    causing edge cases, ugly/hacky code, and the tendency for bugs
    
    The basic idea is that all commands are propagated via alsoPropagate (i.e. added to a list) and the
    top-most call() is responsible for going over that list and actually propagating them (and wrapping
    them in MULTI/EXEC if there's more than one command). This is done in the new function,
    propagatePendingCommands.
    
    Callers to propagatePendingCommands:
    1. top-most call() (we want all nested call()s to add to the also_propagate array and just the top-most
       one to propagate them) - via `afterCommand`
    2. handleClientsBlockedOnKeys: it is out of call() context and it may propagate stuff - via `afterCommand`. 
    3. handleClientsBlockedOnKeys edge case: if the looked-up key is already expired, we will propagate the
       expire but will not unblock any client so `afterCommand` isn't called. in that case, we have to propagate
       the deletion explicitly.
    4. cron stuff: active-expire and eviction may also propagate stuff
    5. modules: the module API allows to propagate stuff from just about anywhere (timers, keyspace notifications,
       threads). I could have tried to catch all the out-of-call-context places but it seemed easier to handle it in one
       place: when we free the context. in the spirit of what was done in call(), only the top-most freeing of a module
       context may cause propagation.
    6. modules: when using a thread-safe ctx it's not clear when/if the ctx will be freed. we do know that the module
       must lock the GIL before calling RM_Replicate/RM_Call so we propagate the pending commands when
       releasing the GIL.
    
    A "known limitation", which were actually a bug, was fixed because of this commit (see propagate.tcl):
       When using a mix of RM_Call with `!` and RM_Replicate, the command would propagate out-of-order:
       first all the commands from RM_Call, and then the ones from RM_Replicate
    
    Another thing worth mentioning is that if, in the past, a client would issue a MULTI/EXEC with just one
    write command the server would blindly propagate the MULTI/EXEC too, even though it's redundant.
    not anymore.
    
    This commit renames propagate() to propagateNow() in order to cause conflicts in pending PRs.
    propagatePendingCommands is the only caller of propagateNow, which is now a static, internal helper function.
    
    Optimizations:
    1. alsoPropagate will not add stuff to also_propagate if there's no AOF and replicas
    2. alsoPropagate reallocs also_propagagte exponentially, to save calls to memmove
    
    Bugfixes:
    1. CONFIG SET can create evictions, sending notifications which can cause to dirty++ with modules.
       we need to prevent it from propagating to AOF/replicas
    2. We need to set current_client in RM_Call. buggy scenario:
       - CONFIG SET maxmemory, eviction notifications, module hook calls RM_Call
       - assertion in lookupKey crashes, because current_client has CONFIG SET, which isn't CMD_WRITE
    3. minor: in eviction, call propagateDeletion after notification, like active-expire and all commands
       (we always send a notification before propagating the command)
    7ac21307
t_stream.c 143 KB