graph-gophers / dataloader

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

wrong time to call 'TraceBatchFinishFunc' #63

Open jimyang112 opened 4 years ago

jimyang112 commented 4 years ago
ctx, finish := b.tracer.TraceBatch(originalContext, keys)
defer finish(items) // this should be moved

func() {
    defer func() {
        if r := recover(); r != nil {
            panicErr = r
            if b.silent {
                return
            }
            const size = 64 << 10
            buf := make([]byte, size)
            buf = buf[:runtime.Stack(buf, false)]
            log.Printf("Dataloader: Panic received in batch function:: %v\n%s", panicErr, buf)
        }
    }()
    items = b.batchFn(ctx, keys)
}()

// defer finish(items) 
mjm commented 4 years ago

I just noticed this too. By calling it here, it captures the wrong items slice, so the finish function from the tracing always gets an empty slice.

nicksrandall commented 3 years ago

PRs are open :)