Recover from panics that happen during fetch #39

edsrzf · 2019-08-11T09:29:07Z

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.

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.

edsrzf · 2019-08-11T09:39:08Z

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.

gracenoah

The test failure is random. It will pass if you re-run the test. It would be good to add a test for this PR though. 1 for no handler and 1 for custom panic handler.

$ go test ./... -test.count 100
?       github.com/vektah/dataloaden    [no test files]
ok      github.com/vektah/dataloaden/example    4.148s
?       github.com/vektah/dataloaden/example/pkgname    [no test files]
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=5) %!s(int=50)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=5) %!s(int=50)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=5) %!s(int=50)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=20) %!s(int=4)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=5) %!s(int=50)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=5) %!s(int=50)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=5) %!s(int=50)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=5) %!s(int=50)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=5) %!s(int=50)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
--- FAIL: TestUserLoader (0.04s)
    --- FAIL: TestUserLoader/it_sent_two_batches (0.00s)
        Error Trace:    usersliceloader_test.go:90
        Error:          "[%!s(int=5) %!s(int=50)]" should have 3 item(s), but has 2
        Test:           TestUserLoader/it_sent_two_batches
FAIL
FAIL    github.com/vektah/dataloaden/example/slice      4.155s
ok      github.com/vektah/dataloaden/pkg/generator      4.264s

gracenoah · 2019-08-11T11:43:19Z

pkg/generator/template.go

 	Fetch func(keys []{{.KeyType.String}}) ([]{{.ValType.String}}, []error)

 	// Wait is how long wait before sending a batch
 	Wait time.Duration

 	// MaxBatch will limit the maximum number of keys to send in one batch, 0 = not limit
 	MaxBatch int
+
+	// Recover is a function to transform a recovered value into an error.
+	// If a function is not supplied, the value is formatted with fmt.Errorf("%v", v).


We should specify that if a function is provided, it must not return nil.

gracenoah · 2019-08-11T11:44:48Z

pkg/generator/template.go

@@ -12,6 +12,7 @@ var tpl = template.Must(template.New("generated").
 package {{.Package}}

 import (
+	"fmt"


Looks like the rest of the imports use 4 spaces rather than tabs. It's not great, but let's stay consistent for now?

edsrzf · 2019-08-12T09:19:39Z

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

gracenoah · 2019-08-12T11:54:07Z

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 · 2020-01-03T12:02:49Z

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)
}

Clark-zhang · 2020-01-03T12:04:59Z

pkg/generator/template.go

+				b.error = []error{fmt.Errorf("%v", r)}
+			}
+		}
+		close(b.done)


this line will cause "panic: close of closed channel", should inside the if-recover case.

Have you tried this code and run into it? The channel needs to always be closed when the function exits. Otherwise Load calls will never return.

We've since forked this code and are running panic-handling almost identical to this in production with no issues.

Hi, i have tried the patch, but I got "panic: close of closed channel".
From my opinion, it's because if l.fetch(b.keys) successfully execute, the channel will be closed, but inside defer func, we close it again. So, the second time we close it, we have to make sure there was panic, means inside r := recover(); && r != nil.

gpopovic · 2020-05-02T11:01:45Z

Any news on this one?

Maleandr · 2021-01-24T14:55:59Z

Can it be merged?

edsrzf added 2 commits August 11, 2019 21:24

Recover from panics that happen during fetch

384c40a

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.

Regenerate examples

5058b1d

gracenoah approved these changes Aug 11, 2019

View reviewed changes

edsrzf added 2 commits August 12, 2019 20:48

Tab -> spaces

3a29fb6

Add tests, fix bug

53233eb

edsrzf mentioned this pull request Aug 19, 2019

Attempt to fix test flakiness #41

Open

Clark-zhang reviewed Jan 3, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recover from panics that happen during fetch #39

Recover from panics that happen during fetch #39

edsrzf commented Aug 11, 2019

edsrzf commented Aug 11, 2019

gracenoah left a comment •

edited

Loading

gracenoah Aug 11, 2019

gracenoah Aug 11, 2019

edsrzf commented Aug 12, 2019

gracenoah commented Aug 12, 2019

Clark-zhang commented Jan 3, 2020

Clark-zhang Jan 3, 2020

edsrzf Jan 6, 2020

Clark-zhang Jan 13, 2020

gpopovic commented May 2, 2020

Maleandr commented Jan 24, 2021

Recover from panics that happen during fetch #39

Are you sure you want to change the base?

Recover from panics that happen during fetch #39

Conversation

edsrzf commented Aug 11, 2019

edsrzf commented Aug 11, 2019

gracenoah left a comment • edited Loading

Choose a reason for hiding this comment

gracenoah Aug 11, 2019

Choose a reason for hiding this comment

gracenoah Aug 11, 2019

Choose a reason for hiding this comment

edsrzf commented Aug 12, 2019

gracenoah commented Aug 12, 2019

Clark-zhang commented Jan 3, 2020

Clark-zhang Jan 3, 2020

Choose a reason for hiding this comment

edsrzf Jan 6, 2020

Choose a reason for hiding this comment

Clark-zhang Jan 13, 2020

Choose a reason for hiding this comment

gpopovic commented May 2, 2020

Maleandr commented Jan 24, 2021

gracenoah left a comment •

edited

Loading