6 ms·

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 hi

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.

keathley 5y ago
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.
- mwcampbell 5y ago
  > 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).
  - dmsnell 5y ago
    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
- jolux 5y ago
  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.
  - keathley 5y ago
    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.
    - 5y ago
bcrosby95 5y ago
Reminds me a bit of this: https://discord.statuspage.io/incidents/62gt9cgjwdgf 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."
dnautics 5y ago
> 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.

keathley 5y ago

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.

mwcampbell 5y ago
> 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).
- dmsnell 5y ago
  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
jolux 5y ago
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.
- keathley 5y ago
  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.
  - 5y ago

bcrosby95 5y ago

Reminds me a bit of this: https://discord.statuspage.io/incidents/62gt9cgjwdgf 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."

dnautics 5y ago

> 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.