vektah / dataloaden

go generate based DataLoader
MIT License
528 stars 79 forks source link

Recover from panics that happen during fetch #39

Open edsrzf opened 5 years ago

edsrzf commented 5 years ago

Now a panic inside fetch doesn't take down the whole process.

This also adds a new optional Recover field to Config structs to allow control over transforming recovered values into errors. When it's nil, fmt.Errorf("%v", v) is used.

Fixes #31.

edsrzf commented 5 years ago

I'm not sure why CI is failing, but I can't reproduce it locally and can't see how it would be related to this change.

edsrzf commented 5 years ago

Added tests and hey, whaddya know, there was a bug.

gracenoah commented 5 years ago

I see one reason these tests are so flaky: they are abusing go's test parallelism. Depending on the number of cores (and therefore default test parallelism), different behaviours will happen. When a test calls t.Parallel(), it blocks and is resumed later. Running the tests with -parallel 2 reproduces the CI failures. I suspect that circle CI used to run on a machine with more cores. For now, try adding -parallel 4 to all go test commands in https://github.com/vektah/dataloaden/blob/master/.circleci/config.yml#L10 . Hopefully that will make the tests pass at least sometimes.

Clark-zhang commented 4 years ago

Hello, guys. If you are going to use this patch, please notice to change close(b.done) line.

func (b *{{.Name|lcFirst}}Batch) end(l *{{.Name}}) {
        defer func() {
                if r := recover(); r != nil {
                        if l.recover != nil {
                                b.error = []error{l.recover(r)}
                        } else {
                                b.error = []error{fmt.Errorf("%v", r)}
                        }
                }
                 //causing panic: close of closed channel. this line should inside if, as the channel will be closed in function end.
                close(b.done)
        }()
        b.data, b.error = l.fetch(b.keys)
        close(b.done)
}

//working version
func (b *{{.Name|lcFirst}}Batch) end(l *{{.Name}}) {
        defer func() {
                if r := recover(); r != nil {
                        if l.recover != nil {
                                b.error = []error{l.recover(r)}
                        } else {
                                b.error = []error{fmt.Errorf("%v", r)}
                        }
                      close(b.done)
                }
        }()
        b.data, b.error = l.fetch(b.keys)
        close(b.done)
}
gpopovic commented 4 years ago

Any news on this one?

Maleandr commented 3 years ago

Can it be merged?