However, I will not call die. I find it frustrating when modules die.
-- Bod in Re^6: STDERR in Test Results
While I doubt Bod, hailing from (working-class) Coventry UK, would be permitted to enter
the hallowed Long Room at Lords
to hurl abuse at the Australian cricket team during the Ashes test match last weekend,
I'm sure he won't be stumped by this meditation's title ...
unlike monks from non-cricket-playing nations, doubtless unfamiliar with Bazball :).
Bodball, you may recall I once scolded you for asking "what should I test?" long after you'd released your module.
I similarly urge you to get into the habit of thinking long and hard about your module's error handling well before you release it,
and for the same reasons.
Like TDD, it's crucial to perform this error-handling analysis early
because doing so will likely change your module's interface.
Further to the excellent general advice you've already recieved from afoken,
I'm interested to learn more about the errors you commonly encounter in practice
when using your Business::Stripe::Webhook module.
I also urge you to add an expanded "Error Handling" section to your module's documentation.
General Error Handling Advice
Don't fail silently.
Failure is inevitable; failing to report failures is inexcusable. Failing silently causes the following problems:
- Users wonder whether something has gone wrong. ("Why did my order not go through?")
- Customer support wonders what caused a problem. ("The log file gave no indication of a problem")
Embrace your software's fallibility. Assume that humans will make mistakes using your software.
Try to minimize ways for people to misuse your software, but assume that you can't completely eliminate misuse.
Therefore, plan error messages as you design software.
-- General error handling advice from Google Error Messages course
Programming Tips
What should a function do if it cannot perform its allocated task?
- return a value indicating failure
- throw an exception
- terminate the program
Return failure when:
- an error is normal and expected (e.g. opening a file)
- an immediate caller can reasonably be expected to handle the failure
Throw an exception when:
- an error is so rare that the programmer is likely to forget to check for it
- an error cannot be handled by the immediate caller
- new kinds of errors are added in lower modules that higher level modules were not written to cope with
- no suitable return path for error codes is available (e.g. semipredicate problem)
- return path of a function is made uglier by the need to return an error indicator
- the function that found the error was a callback
- an error requires an "undo" action (unlike RAII say)
This is not a black and white issue. Experience and good taste are required.
Business::Stripe::Webhook Error Handling
Though unfamiliar with your Business::Stripe::Webhook domain,
I briefly browsed your module's documentation.
Good to see you've already written a short "Errors and Warnings" section in its documentation;
I suggest you improve and expand this section for the next release.
AFAICT, your basic error handling strategy is for your methods to set the error
property, for example:
$vars{'error'} = 'Missing payload data'
with the module user expected to check this error property after calling each method.
Is that right?
I think a clear statement of your overall error-handling strategy, combined
with a couple of real-world examples of handling common errors you've experienced when using your module,
would be invaluable to your users ... and may cause you to tweak your module's error-handling code
and interface ... which is why this step is ideally performed well before release. :)
See Also
Updated: minor changes to wording were made shortly after posting.
Added more references to the See Also section.
|
Re: EyeBall stumps BodBall
by stevieb (Canon) on Jul 07, 2023 at 01:29 UTC
|
There is an overwhelming amount of good advice in this post.
"Don't fail silently."
That can't be stressed enough, especially when dealing with critical data (in this case, financial).
"Like TDD, it's crucial to perform this error-handling analysis early because doing so will likely change your module's interface."
Truer advice has never been given. To me, writing my test platform is the most critical facet to interface design. It dictates it. I'm literally writing tests (nothing more than scripts that are true users of my software) that guides me to the end interface appearance. For me, my error messages (after so many years of coding) are generally logical, but through writing tests to cover edge cases, error messages become more refined, appropriate and understandable.
"with the module user expected to check this error property after calling each method."
Few will do that. You'll receive emails and bug tickets. Then you'll have to hand-hold to get someone to repro the issue which will never be the same again, and the bug goes unfound.
IMHO, a warning is something that you can let your users check for themselves. True errors however should be forefront. A user of the interface should have to do work to work around the error, not hear from someone that something isn't working causing all manner of teams to troubleshoot, then search out the error. Show them up front... somethings broken severely. It might be you, it might be me, but we can't continue until action is taken.
It is counter-productive to force users to go seek out whether there's an error under the hood or not. Break the caller, and allow them to decide whether they want to put an eval{} around it or not.
Failing silently is a nightmare and a frustration across the board.
Magnificent post, eyepopslikeamosquito!
-stevieb
| [reply]
[d/l] |
Re: EyeBall stumps BodBall
by hippo (Archbishop) on Jul 07, 2023 at 07:09 UTC
|
Throw an exception when ... an error is so rare that the programmer is likely to forget to check for it
Yes. Furthermore, I would expand this to include "or you believe that the underlying triggering condition should never occur". I'm sure we have all written code with labyrinthine logical paths and found one branch which should never be reached. In such a scenario I throw an exception in that branch to say as much and briefly indicate why. Such exceptions end up being thrown on a bafflingly frequent basis and this otherwise-unhandled unexpected condition is a really good candidate for an abrupt termination. What else should you do when the logic fails so utterly?
Failing silently causes the following problems: Users wonder whether something has gone wrong. ("Why did my order not go through?")
Much worse than "why didn't it go through?" is "has it gone through or not?" - I've encountered this a few times recently when ordering online and it is frankly infuriating. You end up wrestling not only with the vendor's poor interface but also your card provider's only slightly better one trying to work out if you have been charged zero, one or many times and how many widgets, if any, you can expect to turn up in 5 to 7 working days. Such vendors are migrated swiftly to the bargepole list.
| [reply] |
|
|
> I would expand this to include "or you believe that the underlying triggering condition should never occur"
... What else should you do when the logic fails so utterly?
Yes. You just reminded me of my first boss, great salesman, engaging personality ... still my favourite boss.
Though not a great programmer,
he'd hacked out a system in record time and managed to sell it to a chain of bakeries.
His system made us a lot of money and was working remarkably well until, in the middle of the night,
we get a support call from a very sheepish bakery worker,
reluctant to explain exactly what the problem was.
Finally, she said the system had stopped with "What the f*!!? am I doing here?" displayed on the screen. :)
| [reply] |
Re: EyeBall stumps BodBall (Error Handling)
by eyepopslikeamosquito (Archbishop) on Jul 09, 2023 at 14:01 UTC
|
I'd forgotten to check Perl Best Practices
when writing the root node; from its Error Handling chapter (practices 171-184):
- Throw exceptions instead of returning special values or setting flags
- Make failed builtins throw exceptions too
- Make failures fatal in all contexts
- Be careful when testing for failure of the system builtin
- Throw exceptions on all failures, including recoverable ones
- Have exceptions report from the caller's location, not from the place where they were thrown
- Compose error messages in the recipient's dialect
- Document every error message in the recipient's dialect
- Use exception objects whenever failure data needs to be conveyed to a handler
- Use exception objects when error messages may change
- Use exception objects when two or more exceptions are related
- Catch exception objects in most-derived-first order
- Build exception classes automatically
- Unpack the exception variable in extended exception handlers
See Also
Updated: added more references to the See Also section.
| [reply]
[d/l]
[select] |
|
|
Have exceptions report from the caller's location, not from the place where they were thrown
I think this is the only one that I'd strongly disagree with.
If you get an error thrown deep in some module as a result of a call, it is really easy to use Carp::confess to find out which line of your own code triggered it. However when you know only the line of your own code that triggered it, it can be far harder to diagnose what has actually gone wrong - if you can't immediately determine it based on the documentation of the call you made, you really want to find the line of code that raised the exception and work back from there.
| [reply]
[d/l] |
|
|
I'm sure hv knows this, but for others: if you want to make a die in another module to actually confess, use Devel::Confess, e.g.
perl -d:Confess myfailingscript.pl
| [reply]
[d/l]
[select] |
|
|
Re: EyeBall stumps BodBall
by Bod (Parson) on Jul 08, 2023 at 17:46 UTC
|
I similarly urge you to get into the habit of thinking long and hard about your module's error handling well before you release it, and for the same reasons.
You'll be pleased to hear that I thought at length about error handling before I wrote Business::Stripe::WebCheckout. A consistent approach has been carried over into my other Stripe modules:
Business::Stripe::Subscription
Business::Stripe::Webhook
AFAICT, your basic error handling strategy is for your methods to set the error property
Almost...
The modules set error in any error. The modules provide the success method which returns true on success or false if there was an error. The user of the module is expected to check success. The error method returns the error property so that the user can understand why success is false.
These modules are designed to be used on a webserver. As such, calling die is not helpful to anyone. The end user, the person browsing the website, will probably get an HTTP 500 error and the user of the module is unlikely to detect that quickly.
That's what I meant by: "However, I will not call die. I find it frustrating when modules die"
Instead, I opted for the default error handing approach of DBI. I use this module in default mode (i.e. with RaiseError false) and only turn on RaiseError for debugging, not for live code.
I'm not sure my Stripe modules warrant the equivalent of RaiseError in them. I may be wrong here.
Specifically with Business::Stripe::Webhook, this is designed for building scripts to listen for calls from Stripe to a webhook endpoint. Stripe specifies that the endpoint MUST give a JSON encoded response. The module provides a method for doing this. If the payload from Stripe is invalid or has been changed by the user of the module, then Stripe still needs this reply. If we were to call die then Stripe will not get the reply and may invalidate the endpoint. So not calling die is a very deliberate design decision.
I think a clear statement of your overall error-handling strategy, combined with a couple of real-world examples of handling common errors you've experienced when using your module, would be invaluable to your users
Thank you for this, I shall include it in the next release of each of the Stripe modules.
I work on my own and build modules that are for my use. They only get released if I think they will also be is use for other people. But, like everyone, I suffer from what I call "the curse of knowledge". It is obvious to me what it is supposed to do and how to use it. So it is very difficult to know which bit to explain in full detail and which only need a little explanation.
| [reply]
[d/l]
[select] |
|
|
$self->{'error'} = '';
presumably to avoid confusion from a spurious error property set by an earlier method call. Is that right?
I also noticed your get_subscription method does not do this. Is this a bug?
In case it helps, another minor issue I noticed is the return value of your check_signature method is not documented.
Examining the code indicates it can return 1 or 0 or undef.
It would be good to document this method's return value and the difference between returning 0 and undef.
| [reply]
[d/l]
[select] |
|
|
presumably to avoid confusion from a spurious error property set by an earlier method call. Is that right?
Spot on...
I also noticed your get_subscription method does not do this. Is this a bug?
Well spotted, yes it is a bug. It has been corrected in my code locally. Although I suppose I should have done that on GitHub!
In case it helps, another minor issue I noticed is the return value of your check_signature method is not documented
It helps a lot thank you.
As it says in the documentation, there shouldn't be any need to call this method. Originally I was going to have it as a private method but decided that there may be a use case for checking Stripe signatures outside of the workflow that's documented. So, in the end it became a public method.
The return values have now been added to POD in my local copy.
| [reply]
[d/l]
[select] |
|
|
|
|
| |
|
G'day Bod,
Here's a few things that I do which you may find useful (possibly in code outside the Business::Stripe:: namespace).
In general, these are intended to promote consistency and reduce coding effort.
Have a small number of routines to output messages (e.g. print_error(), print_warning() and so on).
These accept a format for the message along with a variable number of arguments.
Here's a very rough, off-the-top-of-my-head example:
sub print_error {
my ($fmt, @args) = @_;
die 'ERROR: ', sprintf $fmt, @args;
}
Define each format once only.
This could be globally as a constant, specific to a subroutine as a state variable, or using some other method;
you may even use a combination of these.
Here's some arbitrary examples of formats:
'Start date (%s) cannot be after end date (%s)'
'Postcode (%s) contains invalid characters'
'ID is a required field'
You can paste these formats directly into the DIAGNOSTICS (or equivalent) section of your POD.
See perldiag for a multitude of examples of this type of documentation.
Where possible, I try to collect as many problem reports as possible into a single message.
So, if the above three example formats were needed,
the user would know to make changes to date, postcode and ID fields from a single piece of feedback,
rather than being drip-fed one problem at a time.
How messages are presented will very much depend on the interface.
With GUIs (web or other) I often find a small [i] button can provide information
about what exactly is required in a field.
A popup panel can list the problems and also contain the same [i] buttons for ease of reference.
| [reply]
[d/l]
[select] |
|
|
Very helpful, thanks Ken
I have been trying (not always successfully) to have a small number (preferably one) to output everything...this includes a single method as a wrapper around the process method of Template.
It's reassuring to read your advice which is generally aligned with what I'm striving to do.
| [reply]
[d/l] |
|
|
| [reply]
[d/l]
[select] |
|
|
