pipeline panic #208

Activity

1. buraksezer

   self-assigned this

   on Jan 12, 2023
2. buraksezer

   added

   bugSomething isn't workingSomething isn't working

   dmap

   on Jan 12, 2023
3. buraksezer commented on Jan 12, 2023

   

   buraksezer

   on Jan 12, 2023

   Collaborator

   More actions

   Hello @derekperkins,

   I couldn't manage to reproduce the problem. It's very likely there is a nil item in the list of commands. pipe cannot be nil. I reviewed the code. I couldn't find any reason for nil commands. There can be a subtle bug in the command pool logic. Do you use Discard?

   func (dp *DMapPipeline) execOnPartition(ctx context.Context, partID uint64) error {
   	rc, err := dp.dm.clusterClient.clientByPartID(partID)
   	if err != nil {
   		return err
   	}
   	// There is no need to protect dp.commands map and its content.
   	// It's already filled before running Exec, and it's now a read-only
   	// data structure
   	commands := dp.commands[partID]
   	pipe := rc.Pipeline()
   
   	for _, cmd := range commands {
   		pipe.Do(ctx, cmd.Args()...) --> panics
   	}

   Could you share a code snippet to reproduce the problem?
4. buraksezer commented on Jan 12, 2023

   

   buraksezer

   on Jan 12, 2023

   Collaborator

   More actions

   Here is my hypothesis:

   execOnPartition assumes that dp.commands is read-only during the execution. Neither dp.commands nor its content is protected by a mutex during the execution. If your code calls Exec and Discard simultaneously, execOnPartition may encounter nil items in dp.commands.

   Discard calls dp.closedCancel() at line 548, if Exec is already passed the context check at line 503, they can start running parallel.
5. derekperkins commented on Jan 13, 2023

   

   derekperkins

   on Jan 13, 2023

   ContributorAuthor

   More actions

   Yes, we're using Discard and reusing pipelines, but we're not calling Exec and Discard simultaneously on our own accord. This is simplified, but basically this is what we're doing.

   defer pipeline.Discard()
   
   pipeline.Exec(ctx)
   
   for _, futureGet := range futureGets {
       futureGet.Result().Scan()
   }

   I'm wondering if there's a race condition when context is canceled...
6. buraksezer commented on Jan 15, 2023

   

   buraksezer

   on Jan 15, 2023

   Collaborator

   More actions

   I'm wondering if there's a race condition when context is canceled...

   I believe the possible root cause is this.
7. david-sharechat commented on Oct 23, 2024

   

   david-sharechat

   on Oct 23, 2024

   Last edited by david-sharechat

   More actions

   Same issue as #256 - linking for ref.

   I believe it is indeed context cancellation.
   Exec is synchronous except for https://github.com/buraksezer/olric/blob/6ca0e20256e0bd773dcfbbea7933eaa0f05a2f50/pipeline.go#L522-L525

   So context can be cancelled leading to Exec exiting while execOnPartition is still running.

   I don't think this is a great behavior, think it'd be better to maintain sync aspect of Exec by either waiting here:

   err := sem.Acquire(ctx, 1)
   if err != nil {
       // Wait for goroutines to complete
       _ = errGr.Wait()
   	return err
   }

   or alternatively handling cancellation in execOnPartition