mediocregopher / radix

Redis client for Go
MIT License
624 stars 113 forks source link

V4: Use a better go-routine management pattern #281

Open mediocregopher opened 3 years ago

mediocregopher commented 3 years ago

The current pattern that the internal proc package uses is restrictive in a few ways. It would be better to use a pattern more like github.com/oklog/run, which has all go-routines in a group close themselves when one of them exits. I don't think we need to use that actual package, but rather can just fixup proc to use that pattern instead.

nussjustin commented 3 years ago

Just read this issue and #282 and had a random idea, which may or may not be useful:

Go since 1.9 supports profiler labels which can be used to tag goroutines with key=value pairs.

These are meant to help when profiling, but are also used by tools like Goland to label goroutines when debugging.

The last time I tried debugging a test failure in radix at one point I had to switch between goroutines and I wasted time finding the correct since I had to look at each goroutine until I found the correct one. Since I mainly use Goland, having labels could have made this easier, though you can also use them with other editors since this is just a feature of the delve debugger.

Unfortunately these labels are not part of stack traces, though they should be available when looking at crash dumps.

Considering this, what do you think about adding labels to proc goroutines? We could for example require each goroutine to have a "name" label. This way identifying goroutines could be made a lot easier. We could also use this to group multiple goroutines belonging to the same *Proc by giving them an extra label that is unique per *Proc.

Goroutines could also have other, additional labels, though since these are optional, the goroutines could just set these themselves.

mediocregopher commented 3 years ago

That sounds like a good idea, and should be easy enough to incorporate into a new proc package. Thanks for the suggestion :)