top | item 34345906

(no title)

camdencheek | 3 years ago

> The WaitGroup looks suspiciously like errgroup

I heavily used errgroup before creating conc, so the design is likely strongly influenced by that of errgroup even if not consciously. Conc was partially built to address the shortcomings of errgroup (from my perspective). Probably worth adding a "prior art" section to the README, but many of the ideas in conc are not unique.

> In go land, this seems desirable.

I mostly agree, which is why `Wait()` propagates the panic rather than returning it or logging it. This keeps panics scoped to the spawning goroutine and enables getting stacktraces from both the spawning goroutine and the spawned goroutine, which is quite useful for debugging.

That said, crashing the whole webserver because of one misbehaving request is not necessarily a good tradeoff. Conc moves panics into the spawning goroutine, which makes it possible to do things like catch panics at the top of a request and return a useful error to the caller, even if that error is just "nil pointer dereference" with a stacktrace. It's up to the user to decide what to do with propagated panics.

discuss

order

bertmuthalaly|3 years ago

A "prior art" section is especially useful for people evaluating your library!

preseinger|3 years ago

> That said, crashing the whole webserver because of one misbehaving request is not necessarily a good tradeoff. Conc moves panics into the spawning goroutine, which makes it possible to do things like catch panics at the top of a request and return a useful error to the caller, even if that error is just "nil pointer dereference" with a stacktrace. It's up to the user to decide what to do with propagated panics.

The problem is that panics aren't "goroutine scoped" in terms of their potential impact. So it really shouldn't be up to the user to decide how to handle a panic. Application code shouldn't handle panics at all! They're not just a different way to yield an error, they're critical bugs which shouldn't occur at all.

camdencheek|3 years ago

> panics aren't "goroutine scoped" in terms of their potential impact

I'm with ya there. However, there are also many classes of logic errors that are not goroutine-scoped. And there are many panics that do not have impact outside of the goroutine's scope. In my experience, this is true of most panics.

In practice, panics happen. They are (almost) always indicative of a bug, and almost always mean there is something that needs fixed. However, if a subsystem of my application is broken and panicking, there's a pretty good chance that reporting the panic without crashing the process will provide a better end user experience than just blowing up.

Yes, that means I'm accepting the risk that my application is left in an inconsistent state, but coupled with good observability/reporting, that's a tradeoff I'm willing to make.

(bonus: this is especially true when propagating panics allow me to capture more debugging information to fix the panics faster)

BreakfastB0b|3 years ago

I would agree if it weren’t super easy to cause a panic in go.

Index slice out of bounds? panic. Close a channel twice? Panic. Incorrect type assertion? Panic. Dereference nil pointer? Panic.

I would argue that all of these examples which are the most common in my experience are “goroutine scoped” because the goroutine was aborted before they potentially modified the application state in an unknown way.

It’s like not in C, or C++ where out of bounds access has now put the entire application into an unknown state.

jasonhansel|3 years ago

If your programming language handles very common errors by crashing the entire application, and if preventing these crashes is actively discouraged, then that suggests a flaw in the language itself.

This would be fine for a low-level language like C where you need to allow SEGFAULTs, but designing it into a high-level language makes no sense.

morelisp|3 years ago

> This keeps panics scoped to the spawning goroutine

This is exactly what's undesirable. (IMO and from my reading the GP agrees.)