Comment by losvedir | Hacker News Reader

Comment by losvedir | Hacker News Reader

Parent Full thread

I dunno, I rarely get issues that crash just once. For example, in the blog post, it's recommended to use `Jason.decode!`. But what if whatever server you're hitting is returning invalid JSON for a time? If you crash, the GenServer will restart, it will crash again, and it will just be either in a crash loop (meanwhile DOS-ing the server?), or worse - if it happens in `init` - will take down its own supervisor after a number of failures, cascading up the tree until your app crashes and restarts.

I think it's better to handle the Jason.decode error, and apply some sort of backoff strategy on re-requests. In general, you have so little control over the timing and count and rate of restarts. It's a very blunt object to just crash. Again, with the `Jason.decode!` error, suppose it's in a GenServer that's updating its state. In some cases you may want to reset the state to empty (essentially what crashing does) and in other situations you may want to keep the previous state, from the last time you made that request. Maybe you want to track how many consecutive failures you're seeing and maintain the state for some time, but eventually blank it, or crash at that point.

It could very well depend on the problem domain, though, for sure. I mostly work on freestanding, pure Elixir apps, and a couple of Phoenix apps. I'm more thinking about the freestanding apps, since in Phoenix, sure, just crash your request, whatever.

You can certainly invent a scenario where using `Jason.decode!` wouldn't be appropriate. In that scenario, absolutely, handling the error and backing off is more appropriate. I'd also argue you shouldn't be doing side-effects like that in an init without a lot of care as well.

The same would be true if you were building a kafka consumer. You wouldn't want to crash in that scenario since you could easily poison the entire topic with one bad message.

There are ways to allow for crashes this way though. An alternative approach would be to use a dedicated process for scheduling work and a dynamic supervisor that can be used to start workers as needed. The work scheduler would monitor these worker processes. This means that you could freely crash the worker and allow the work scheduler to determine what further action must be taken. I've used both your approach, and this alternative approach in the past both to good effect.

> I'd also argue you shouldn't be doing side-effects like that in an init without a lot of care as well.

I learned this the hard way. I wrote a GenServer that does some periodic database cleanup (maybe not the best solution), and I naively wrote the init function to immediately do the first cleanup on startup. Then I discovered a case off the happy path where the cleanup was failing due to a constraint violation, and the failing init brought down the whole application (edit: while someone was trying to use it, of course).

yep - init is for things you can guarantee, not things you can't, like anything needing to make network or database calls.

handle_continue was added for these kinds of things that should happen when starting up gen_ processes.

the entire init chain is sequential, so not only will it risk crashing your entire supervisor tree, but it will slow startup

The scenario is not invented per se, it’s something that really happens sometimes. Maybe the systems you interact with never return malformed data more than a few times, but the ones I work with do. Throwing an exception on a deserialization failure in order to crash your process just seems like punting on your retry logic to me.

Sorry, "invent" had the wrong connotations there. I just meant that there are scenarios where crashing wouldn't be acceptable. And in that case, sure, you should prefer to handle the error directly.

But, allowing the process to crash doesn't punt retry logic at all. For instance, in the work scheduler example, the work scheduler would detect that a worker had crashed and would back off appropriately. This allows you to write your worker code in a way that avoids error handling while still managing faults.

The work scheduler example involves writing a work scheduler, though :). The logic is going to go somewhere, is what I’m saying.

Right, but often you should punt your retry logic. You already need to handle process crash at high level, so you already want some kind of restart/backoff in your supervisor. Unless there's something specific that you want to do differently for this particular kind of failure, having separate retry/backoff logic for each kind of failure is just clutter.

You should definitely handle the crash at a high level, but that’s not the same as punting on it!

Not putting any explicit handling for failed requests and instead allowing the current process to crash, knowing that the supervisor's handling will do the right thing in that case, isn't that punting it? I'm not fluent in American but doesn't punting pretty much mean kicking something far away?

It means giving up, more or less. The default handling that a supervisor provides is probably not sufficient for a looping failure, was the point.

> I think it's better to handle the Jason.decode error, and apply some sort of backoff strategy on re-requests.

What's keeping you from implementing a back off strategy in your init? You can return :ignore to the supervisor and try again later.

If you're backing off inside the process itself, you're gonna get some weird shit where your service exists but it's not in a truly available state, and you will have to handle that by basically halfheartedly rewriting logic that already exists in OTP.

Reminds me a bit of this: https://discord.statuspage.io/incidents/62gt9cgjwdgf

In particular: "14:55 - Engineers pinpoint the issue to be strongly correlated to a spike in errors in originating from our service discovery modules. It is determined that the service discovery processes of our API service had gotten into a crash loop due to an unexpected deserialization error. This triggered an event called "max restart intensity" where, the process's supervisor determined it was crashing too frequently, and decided to trigger a full restart of the node. This event occurred instantaneously across approximately 50% of the nodes that were watching for API nodes, across multiple clusters. We believed it to be related to us hitting a cap in the number of watchers in etcd (the key-value store we use for service discovery.) We attempt to increase this using runtime configuration. Engineers continue to remediate any failed nodes, and restore service to our users."

Reply on news.ycombinator.com