graph-gophers / dataloader

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

Improve panic handling #39

Closed dvic closed 6 years ago

dvic commented 6 years ago

What do you think about improving the panic handling? I was thinking of a custom handler or passing an error to all results in case of a panic? Or do you believe one should do this in the batchFunc itself?

nicksrandall commented 6 years ago

We currently catch panics that happen in the batch function and pass the error to all results. The relative code can be found here: https://github.com/nicksrandall/dataloader/blob/v5/dataloader.go#L419-L441

What are you proposing we do on top of that?

dvic commented 6 years ago

Ah missed that part below it, then there is something up in my tests (using gomock), the test hangs when a gomock panic occurs. I'll investigate and report back if there were any issues related to this library.

&Result{Error: fmt.Errorf("Panic received in batch function: %v", panicErr)}

An improvement to the library might be to be able to configure a 'panic handler', i.e., a func(panicErr interface{}) Result. But this is of course a minor improvement.

nicksrandall commented 6 years ago

Consumers of this library may implement their own panic handler inside the batch function if they wish to. As such, I'm not convinced that we need such a feature baked into the library.