checked constructor for potential error conditions #4
Conversation
This commit introduces a variant of the NewChooser constructor that will error on conditions that could later cause a runtime issue during Pick(). The conditions handled are a lack of valid choices and a potential integer overflow in the running total. This is a proof of concept, but the final API may likely be different to avoid introducing extra complexity into the library. This commit merely serves as the intial code to aide a discussion in the PR.
|
Pinging @0xERR0R since from a quick GitHub search, your Blocky project appears to be the current largest public project utilizing this library. Pinging @mccutchen since I noticed you starred this repo recently and we've worked together in the past. 😁 |
|
I would prefer the option 3. It is always better to check all errors earlier and fail fast and cheap (and not at runtime). |
I don't see a current use case scenario where being able to act upon these as sentinel errors would be significant, so better to avoid the API complexity and keep them private for now. Always easier to export them later if needed than taking them away once out in the wild.
Codecov Report
@@ Coverage Diff @@
## master #4 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 22 27 +5
=========================================
+ Hits 22 27 +5
Continue to review full report at Codecov.
|
|
@0xERR0R potentially up for a quick code review on this before merge? I'm thinking this could be targeted for a shortly upcoming |
This commit introduces a variant of the
NewChooserconstructor that will pre-check for and error on conditions that could later cause a runtime issue duringPick.The conditions handled are a lack of valid choices (fairly preventable by consumers) and a potential integer overflow in the running total (far more subtle, but an edge case). In general, it is highly preferable for us to expose these predictable scenarios rather than potentially allowing a runtime error that could panic or produce invalid results. I was thinking of this Cloudflare blog post as a similar scenario when proposing these changes.
The initial version of this is API backwards compatible by introducing a new
NewChooserCheckedfunction and changing the behavior of theNewChoosertopanicat construction time in invalid states. However I'd like to iterate towards a more simple API before making this change. Some options:Expose both
NewChooserandNewChooserChecked(as in the current proof of concept commit). I am not generally in favor of this as it introduces more user-complexity than needed and the distinction here is not significantly subtle to require the cognitive overhead of both options.Privatize
newChooserCheckedand keep the panic on error behavior onNewChooser. Pros: API backwards compatibility (though pre-release, so not as important), simpler caller semantics, the error conditions are currently fairly edge and unlikely to be encountered in typical usage. Cons: doesn't force error handling, and the overflow error scenario in particular, being an edge case, is unlikely to be pre-checked by consumers of the API, and panic recovery is a pain.Make
NewChooserCheckedthe newNewChooser(renaming/replacing it), which introduces an API change in the return value of the constructor to(*Chooser, error). This would require a semantic version bump, but we're still in pre-release versioning so it makes sense to iterate towards the most ideal API now while we have the flexibility. Pros: Forces dealing with the potential error condition. Cons: Forces slightly more complex initialization code everywhere (e.g. checking err), current error conditions are not incredibly likely to emerge in typical usage.Currently my current leaning is towards option number 3 (especially since the potentially use cases of this library include problem domains with highly reliable characteristics are desirable, such as load-balancing) but I would love to hear from actual users of this library. (To best have an informed opinion, I suggest reviewing the actual source code changes so you can see how likely you are to encounter the potential error conditions in your own usage.)
Some additional API considerations:
ErrWeightOverflow) or not (currently exported in initial commit). My initial inclination is that exporting them complicates the external surface of the API with minimal benefit, and I cannot currently conceive of a situation where a caller fo the constructor would act upon different error conditions significantly differently. (please let me know if you can think of a need here!).