3 ms·

Yeah, this is reliability engineering 101 - don't ever expect that outside content won't be malformed, even from 'trusted' systems. There was a codepath somewhe

by q3k 7y ago

Yeah, this is reliability engineering 101 - don't ever expect that outside content won't be malformed, even from 'trusted' systems. There was a codepath somewhere that went from an unmarshal error into a crash - and that's something that should not pass any code review.

toast0 7y ago
In Erlang (and Elixir) systems, turning unexpected errors into crashes is what you're supposed to do. Unfortunately, it's pretty easy for that to spiral into node shutdown/restarts, everybody ends up learning that one the hard way. Supervision trees really do need to be considered, but what are you expected to do when the service discovery system is unreachable, broken, or filled with bad data? There's certainly options, giving up isn't necessarily the wrong one.
- jchw 7y ago
  This may be true, but malformed input that is unchecked is not, imo, an “exceptional” case but nominally expected, and kind of dangerous since it is one of those things that isn’t likely to happen often since it requires another bug or fault to reveal itself. It’s probably an unpopular opinion at this point but I definitely appreciate Go’s approach to error handling here. Error cases are in-your-face by default for the most part, leaving nearly only programming errors and critical faults to panic ideally.
  - toast0 7y ago
    The important idea of the Erlang approach is not to leave things unchecked; the idea is that the proper response to most errors is to restart from a known good configuration. Of course, restarting the service discovery client may not help, especially when the service discovery system's state is stable but broken. I don't know what you're supposed to do if you're dependent on services, and the service discovery system you use to find the services is not reachable or is returning garbage information. Better logging is always nice, but you can't serve the traffic.
- q3k 7y ago
  > what are you expected to do when the service discovery system is unreachable, broken, or filled with bad data? Different things based on which one of these actually happened! I would assume the code that caused this issue was along the lines of: var addresses []string{} for k, el := range etcdEntries { var element data.Service addr, err := json.Unmarshal(&element, el) if err != nil { return fmt.Errorf("could not unmarshal service entry %q: %w", k, err) } addresses = append(addresses, addr) } And the issue is the lack of graceful degradation, in the form of not failing the entire conversion if one entry is broken. Instead, this should be something like: var addresses []string{} for k, el := range etcdEntries { var element data.Service addr, err := json.Unmarshal(&element, el) if err != nil { metrics.ServiceEntryBroken("key", k).Up() glog.Errorf("Could not unmarshal service entry %q: %v", k err) continue } addresses = append(addresses, addr) } Of course, this takes much more effort. But such is the nature of writing high quality, reliable software. Naturally, if your entire SD service was down, you would fail the entire thing much earlier. That is indeed the thing about Go errors being plain variables - you are intended to write error handling logic (or at least are forced to consider it), not just pack the buck down by default, via either optional types or exceptions.

by q3k 7y ago

toast0 7y ago

In Erlang (and Elixir) systems, turning unexpected errors into crashes is what you're supposed to do. Unfortunately, it's pretty easy for that to spiral into node shutdown/restarts, everybody ends up learning that one the hard way. Supervision trees really do need to be considered, but what are you expected to do when the service discovery system is unreachable, broken, or filled with bad data? There's certainly options, giving up isn't necessarily the wrong one.

jchw 7y ago
This may be true, but malformed input that is unchecked is not, imo, an “exceptional” case but nominally expected, and kind of dangerous since it is one of those things that isn’t likely to happen often since it requires another bug or fault to reveal itself. It’s probably an unpopular opinion at this point but I definitely appreciate Go’s approach to error handling here. Error cases are in-your-face by default for the most part, leaving nearly only programming errors and critical faults to panic ideally.
- toast0 7y ago
  The important idea of the Erlang approach is not to leave things unchecked; the idea is that the proper response to most errors is to restart from a known good configuration. Of course, restarting the service discovery client may not help, especially when the service discovery system's state is stable but broken. I don't know what you're supposed to do if you're dependent on services, and the service discovery system you use to find the services is not reachable or is returning garbage information. Better logging is always nice, but you can't serve the traffic.
q3k 7y ago
> what are you expected to do when the service discovery system is unreachable, broken, or filled with bad data? Different things based on which one of these actually happened! I would assume the code that caused this issue was along the lines of: var addresses []string{} for k, el := range etcdEntries { var element data.Service addr, err := json.Unmarshal(&element, el) if err != nil { return fmt.Errorf("could not unmarshal service entry %q: %w", k, err) } addresses = append(addresses, addr) } And the issue is the lack of graceful degradation, in the form of not failing the entire conversion if one entry is broken. Instead, this should be something like: var addresses []string{} for k, el := range etcdEntries { var element data.Service addr, err := json.Unmarshal(&element, el) if err != nil { metrics.ServiceEntryBroken("key", k).Up() glog.Errorf("Could not unmarshal service entry %q: %v", k err) continue } addresses = append(addresses, addr) } Of course, this takes much more effort. But such is the nature of writing high quality, reliable software. Naturally, if your entire SD service was down, you would fail the entire thing much earlier. That is indeed the thing about Go errors being plain variables - you are intended to write error handling logic (or at least are forced to consider it), not just pack the buck down by default, via either optional types or exceptions.