fix: avoid deadlock in indexer when processor errors #407
Conversation
| @@ -432,7 +432,7 @@ func (t *TipSetIndexer) runActorProcessor(ctx context.Context, p ActorProcessor, | |||
| } | |||
| } | |||
|
|
|||
| func (t *TipSetIndexer) Close() error { | |||
| func (t *TipSetIndexer) closeProcessors() error { | |||
There was a problem hiding this comment.
Commentary: Should we be returning the errors in addition to logging them, or should we drop the return param from both of these close methods?
There was a problem hiding this comment.
I think in this case we want to log the errors because we want to continue with all the close calls, not exit at first error.
We could remove the error return. I tend to keep them assuming that one day we will return an error and the caller should handle it. But that's not a very strong argument to keep it. What do you think?
There was a problem hiding this comment.
I'd be in favor of nixing the errors, it makes the code a bit more readable -- but these aren't strong feelings. I'll leave it up to you.
| return nil | ||
| } | ||
|
|
||
| func (t *TipSetIndexer) Close() error { | ||
| // ensure there are no persist go routines left running | ||
| t.persistSlot <- struct{}{} |
There was a problem hiding this comment.
Im wondering if:
if len(t.persistSlot) == 0 {
return
} else {
t.persistSlot <- struct{}{}
}might be safer?
For example, if the context is canceled, I'm not sure the channel will be emptied.
There was a problem hiding this comment.
That's potentially racy. Another way is to include a timeout:
select {
case t.persistSlot <- struct{}{}:
case time.After(3*time.Second):
}There was a problem hiding this comment.
I think what's here is fine for now. I don't have enough inside into the time it takes to persist models to make an accurate guess on an appropriate timeout.
No description provided.