bitlair/revbank
6
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Ignore all hook exceptions except in hook_checkout_prepare

Browse source 
A space had a custom plugin that died during hook_checkout, which caused
the CHECKOUT lines to be logged without the corresponding BALANCE, and
indeed no account balances were updated. While the plugin had a bug, it
should not cause a half transaction in RevBank.

After some hesitation, I went with ON ERROR RESUME NEXT because if a
hook throws an exception, that should not interfere with other plugins
(the hook can return ABORT if this it was intentional), including the
calling plugin. An error message is printed (but not logged... TODO: add
hook_plugin_fail to plugins/log) but the show must go on.

During hook_checkout_prepare, however, nothing is set in stone yet, so
this could be used for something that might die, and this instance of
call_hooks() is now the one place where a failing hook should result in
the transaction getting aborted. For this, call_hooks() now returns a
success status boolean. Maybe it would make sense in more places, but I
didn't identify any such calls yet.

RevBank::Cart->checkout used to return a success status boolean, but it
could just as well just die (indirectly, to abort the transaction) since
it can't be called a second time within the same transaction anyway
(because ->set_user must be called exactly once), so continuing with the
same transaction can't result in anything useful anyway.

In some places, error messages were slightly improved to contain a bit
more information.
This commit is contained in:
Juerd Waalboer 2023-11-24 04:56:17 +01:00
parent 0c2b24bdc1
commit 52749df5f3
6 changed files with 27 additions and 20 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
lib/RevBank/Cart.pm
View file

@ -78,9 +78,8 @@ sub size($self) {
sub checkout($self, $user) {
if ($self->entries('refuse_checkout')) {
warn "Refusing to finalize deficient transaction.\n";
$self->display;
return;
die "Refusing to finalize deficient transaction";
}
$user = RevBank::Users::assert_user($user);
@ -108,7 +107,9 @@ sub checkout($self, $user) {
$transaction_id = time() - 1300000000;
}
RevBank::Plugins::call_hooks("checkout_prepare", $self, $user, $transaction_id);
RevBank::Plugins::call_hooks("checkout_prepare", $self, $user, $transaction_id)
or die "Refusing to finalize after failed checkout_prepare";
for my $entry (@$entries) {
$entry->sanity_check;
$entry->user($user) if not $entry->user;
@ -135,11 +136,9 @@ sub checkout($self, $user) {
RevBank::Plugins::call_hooks("checkout_done", $self, $user, $transaction_id);
sleep 1; # look busy
$self->empty;
};
$self->empty;
return 1;
}
sub entries($self, $attribute = undef) {

2
lib/RevBank/FileIO.pm
View file

@ -73,7 +73,7 @@ sub with_lock :prototype(&) ($code) {
$rv = $code->() if not $list_context;
};
release_lock;
croak $@ =~ s/\n$/, called/r if $@;
croak $@ =~ s/\.?\n$/, rethrown/r if $@;
return @rv if $list_context;
return $rv if not $list_context;
}

14
lib/RevBank/Plugins.pm
View file

@ -20,17 +20,25 @@ sub _read_file($fn) {
sub call_hooks {
my $hook = shift;
my $method = "hook_$hook";
my $success = 1;
for my $class (@plugins) {
if ($class->can($method)) {
my ($rv, @message) = $class->$method(@_);
my ($rv, @message) = eval { $class->$method(@_) };
if (defined $rv and ref $rv) {
if ($@) {
$success = 0;
call_hooks("plugin_fail", $class->id, "$class->$method died: $@");
} elsif (defined $rv and ref $rv) {
main::abort(@message) if $rv == ABORT;
warn "$class->$method returned an unsupported value.\n";
$success = 0;
call_hooks("plugin_fail", $class->id, "$class->$method returned an unsupported value");
}
}
}
return $success;
};
sub register(@new_plugins) {

2
lib/RevBank/Plugins.pod
View file

@ -158,7 +158,7 @@ Be careful to avoid infinite loops if you add new stuff.
=item hook_checkout_prepare($class, $cart, $user, $transaction_id, @)
Called when the transaction is about to be processed. In this phase, the cart and its entries can still be manipulated.
Called when the transaction is about to be processed. In this phase, the cart and its entries can still be manipulated. If the hook throws an exception, the transaction is aborted.
=item hook_checkout($class, $cart, $user, $transaction_id, @)

2
plugins/users
View file

@ -17,7 +17,7 @@ sub command :Tab(list,ls,shame,log,USERS) ($self, $cart, $command, @) {
return $self->balance($user) if not $cart->size;
$cart->checkout($user) or return REJECT, "Checkout failed.";
$cart->checkout($user);
return ACCEPT;
}

14
revbank
View file

@ -17,7 +17,7 @@ use RevBank::Global;
use RevBank::Messages;
use RevBank::Cart;
our $VERSION = "4.2.1";
our $VERSION = "4.2.2";
our %HELP1 = (
"abort" => "Abort the current transaction",
);
@ -183,18 +183,18 @@ OUTER: for (;;) {
ALL_PLUGINS: { PLUGIN: for my $plugin (@plugins) {
my ($rv, @rvargs) = eval { $plugin->$method($cart, $word) };
if ($@) {
call_hooks "plugin_fail", $plugin->id, $@;
call_hooks "plugin_fail", $plugin->id, "$method: $@";
abort;
}
if (not defined $rv) {
call_hooks "plugin_fail", $plugin->id, "No return code";
call_hooks "plugin_fail", $plugin->id, "$method: No return code";
abort;
}
if (not ref $rv) {
$prompt = $rv;
@plugins = $plugin;
($method) = @rvargs;
call_hooks "plugin_fail", $plugin->id, "No method supplied"
call_hooks "plugin_fail", $plugin->id, "$method: No method supplied"
if not ref $method;
abort "Incomplete command." if $one_off and not @words;
@ -231,11 +231,11 @@ OUTER: for (;;) {
}
if ($rv == NEXT) {
next PLUGIN if $method eq 'command';
call_hooks "plugin_fail", $plugin->id, "Only 'command' "
. "should ever return NEXT.";
call_hooks "plugin_fail", $plugin->id, "$method: "
. "Only 'command' should ever return NEXT.";
abort;
}
call_hooks "plugin_fail", $plugin->id, "Invalid return value";
call_hooks "plugin_fail", $plugin->id, "$method: Invalid return value";
abort;
}
call_hooks "invalid_input", $cart, $origword, $word, \@allwords;