graph-gophers / dataloader

Implementation of Facebook's DataLoader in Golang
MIT License
1.2k stars 75 forks source link

Preventing panic after cancelled context #98

Closed josgba closed 1 year ago

josgba commented 1 year ago

Hello!

I'm noticing that when request context is cancelled during operation, the thunk related to that request panics when it is resolved.

This seems to be due to the thunk not having a value property when the return value in this block is hit (in dataloader.go):

thunk := func() (interface{}, error) {
    result.mu.RLock()
    resultNotSet := result.value == nil
    result.mu.RUnlock()

    if resultNotSet {
        result.mu.Lock()
        if v, ok := <-c; ok {
            result.value = v
        }
        result.mu.Unlock()
    }
    result.mu.RLock()
    defer result.mu.RUnlock()
    return result.value.Data, result.value.Error
}

When this thunk is cached, it then results in a panic in subsequent requests until the cache is cleared.

Is there a suggested way to guard against this? I've tried the following in my cache implementation to attempt to guard against caching these 'invalid' thunks, but it results in a deadlock:

func (c *Cache) Set(ctx context.Context, key dataloader.Key, value dataloader.Thunk) {
    defer func() {
        if r := recover(); r != nil {
            c.C.Clear()
        }
     }()
     value()
    c.C.SetWithTTL(key.String(), value, 0, time.Duration(c.MaxAge*int(time.Hour)))
}
vivek-ng commented 1 year ago

I merged a fix for this back in April but we haven't published a new version 😞 https://github.com/graph-gophers/dataloader/blob/69412165eb30a45d7933e953c719217a2a25c96a/dataloader.go#L235

@tonyghita @pavelnikolov Can we please publish a new version?

pavelnikolov commented 1 year ago

Thank you @vivek-ng I just cut a new release version https://github.com/graph-gophers/dataloader/releases/tag/v7.1.0