1Password
feat: return errors in exported functions instead of panicking
👋🏽 hi team
What
This PR proposes braking changes to some of the exported functions ⚠️
Namely, NewSRPClient(), NewSRPServer(), and KDFRFC5054().
Concretely, it changes the signature of these methods so that they return an error (a common Go idiom), rather than panicking. This allows consumers of the library to handle the errors gracefully and at their convinience.
What approach did you choose and why?
I identified all the places where we panic. I refactored it to emit an error instead. I then worked my way upwards so that the error gets bubbled up all the way to the exported functions.
What should reviewers focus on?
I believe there is much to be gained from this breaking change:
- a review of the release process for this library: from what I can tell there are no release docs
- adhering to one of the core Go idioms: don't panic
So the questions I think we should focus on are (if the changes are approved):
- how are we going to roll out this change?
- do we want to notify public consumers of this library?
If something goes wrong, what are the mitigation strategies?
Consumers of this library can use the previous tag or a commit SHA prior to this PR.
Note that I am not an official collaborator on this repo, so my commits won't trigger CI builds. If you want to see the CI for these commits, see my fork: https://github.com/CamiloGarciaLaRotta/srp/pull/2/commits
Performance Impact
None
Observability
N/A
You are absolutely right about this. I do not recall why I choose to panic on those in the first place. It may have been that I was trying to maintain some compatibility with how these were being called by other code.
This would be a "breaking change", so it needs to be tested carefully in the code that makes use of this package.
Just to point it out:
adhering to one of the core Go idioms: don't panic
The "don't panic" idiom says (emphasis added):
Don't use panic for normal error handling. Use error and multiple return values.
Events like failure to fetch random bytes from the system source isn't really a normal failure, and indicates the system is in a pretty bad state. For exceptional circumstances like that (where there is truly nothing we can do to proceed), I think panic is quite appropriate. Equally, sometimes we know that something is going to succeed (e.g. the hash writers we use don't return errors under any circumstance, but it's part of the signature to fulfil the writer API contract. There isn't really much value in making callers deal with a potential error that can't actually occur, so panic is also probably okay here because we are asserting we know that a write to the hash writer can't fail (not the case for all writers, but for the hash writer it is).
Agreed that for simpler error handling, we should not be panicing though.
