Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Improve failed change log error handling.

I was getting a failure that just said the transaction was rolled back, with
no notice of the original error. This was happening because the deploy script
properly ran, but Sqitch's logging of the change failed.

So we do exception handling around the logging of the change. If it dies,
Sqitch now vents the underlying error, tells the engine to roll back work, and
then runs the revert script so as to restore the database to its original
state.
  • Loading branch information...
commit bd7aeb2997efcca52af6868e8dc5b898b2aede54 1 parent 1626ed7
@theory authored
View
5 Changes
@@ -18,6 +18,11 @@ Revision history for Perl extension App::Sqitch
allows it (issue #55). Thanks to @user5402 in [this SO
answer](http://stackoverflow.com/a/13370233/79202) for the solution.
- Fixed bug where the `core.plan_file` config variable was ignored.
+ - Improved error handling when deploying and reverting a change. If the
+ change successfully deployed but the logging of the deployment to the
+ database failed, there was just a rollback message. Sqitch will now
+ emit the underlying error *and* run the revert script for the
+ just-deployed change.
0.938 2012-10-12T19:16:57Z
- Added a primary key to the PostgreSQL `events` table, which should make
View
55 lib/App/Sqitch/Engine.pm
@@ -291,7 +291,7 @@ sub deploy_change {
my ( $self, $change ) = @_;
my $sqitch = $self->sqitch;
$sqitch->info(' + ', $change->format_name_with_tags);
- $self->begin_work;
+ $self->begin_work($change);
# Check for conflicts.
if (my @conflicts = grep {
@@ -319,19 +319,36 @@ sub deploy_change {
return try {
$self->run_file($change->deploy_file);
- $self->log_deploy_change($change);
+ try {
+ $self->log_deploy_change($change);
+ } catch {
+ # Oy, our logging died. Rollback.
+ $sqitch->vent(eval { $_->message } // $_);
+ $self->rollback_work($change);
+
+ # Begin work and run the revert.
+ try {
+ $self->sqitch->info(' - ', $change->format_name_with_tags);
+ $self->begin_work($change);
+ $self->run_file($change->revert_file);
+ } catch {
+ # Oy, the revert failed. Just emit the error.
+ $sqitch->vent(eval { $_->message } // $_);
+ };
+ hurl deploy => __ 'Deploy failed';
+ };
} finally {
- $self->finish_work;
+ $self->finish_work($change);
} catch {
$self->log_fail_change($change);
die $_;
- }
+ };
}
sub revert_change {
my ( $self, $change ) = @_;
$self->sqitch->info(' - ', $change->format_name_with_tags);
- $self->begin_work;
+ $self->begin_work($change);
if (my @requiring = $self->changes_requiring_change($change)) {
my $proj = $self->plan->project;
@@ -350,16 +367,24 @@ sub revert_change {
try {
$self->run_file($change->revert_file);
- $self->log_revert_change($change);
+ try {
+ $self->log_revert_change($change);
+ } catch {
+ # Oy, our logging died. Rollback and revert this change.
+ $self->sqitch->vent(eval { $_->message } // $_);
+ $self->rollback_work($change);
+ hurl revert => 'Revert failed';
+ };
} finally {
- $self->finish_work;
+ $self->finish_work($change);
} catch {
die $_;
- }
+ };
}
sub begin_work { shift }
sub finish_work { shift }
+sub rollback_work { shift }
sub earliest_change {
my $self = shift;
@@ -698,19 +723,27 @@ These methods must be overridden in subclasses.
=head3 C<begin_work>
- $engine->begin_work;
+ $engine->begin_work($change);
This method is called just before a change is deployed or reverted. It should
create a lock to prevent any other processes from making changes to the
-database, to be freed in C<finish_work>.
+database, to be freed in C<finish_work> or C<rollback_work>.
=head3 C<finish_work>
- $engine->finish_work;
+ $engine->finish_work($change);
This method is called after a change has been deployed or reverted. It should
unlock the lock created by C<begin_work>.
+=head3 C<rollback_work>
+
+ $engine->rollback_work($change);
+
+This method is called after a change has been deployed or reverted and the
+logging of that change has failed. It should rollback changes started by
+C<begin_work>.
+
=head3 C<initialized>
$engine->initialize unless $engine->initialized;
View
6 lib/App/Sqitch/Engine/pg.pm
@@ -300,6 +300,12 @@ sub finish_work {
return $self;
}
+sub rollback_work {
+ my $self = shift;
+ $self->_dbh->rollback;
+ return $self;
+}
+
sub run_file {
my ($self, $file) = @_;
$self->_run('--file' => $file);
View
19 t/engine.t
@@ -4,7 +4,7 @@ use strict;
use warnings;
use 5.010;
use utf8;
-use Test::More tests => 255;
+use Test::More tests => 256;
#use Test::More 'no_plan';
use App::Sqitch;
use App::Sqitch::Plan;
@@ -562,6 +562,7 @@ is_deeply $engine->seen, [
[run_file => $changes[3]->deploy_file],
[run_file => $changes[4]->deploy_file],
[run_file => $changes[5]->deploy_file],
+ [run_file => $changes[5]->revert_file],
[log_fail_change => $changes[5] ],
[changes_requiring_change => $changes[4] ],
[run_file => $changes[4]->revert_file],
@@ -576,11 +577,13 @@ is_deeply +MockOutput->get_info, [
[' + ', 'lolz'],
[' + ', 'tacos'],
[' + ', 'curry'],
+ [' - ', 'curry'],
[' - ', 'tacos'],
[' - ', 'lolz'],
], 'Should have seen deploy and revert messages';
is_deeply +MockOutput->get_vent, [
['ROFL'],
+ [__ 'Deploy failed'],
[__x 'Reverting to {target}', target => 'widgets @beta']
], 'The original error should have been vented';
$mock_whu->unmock('log_deploy_change');
@@ -720,6 +723,7 @@ is_deeply $engine->seen, [
[run_file => $changes[0]->deploy_file],
[run_file => $changes[1]->deploy_file],
[run_file => $changes[2]->deploy_file],
+ [run_file => $changes[2]->revert_file],
[log_fail_change => $changes[2]],
[changes_requiring_change => $changes[1] ],
[run_file => $changes[1]->revert_file],
@@ -733,12 +737,14 @@ is_deeply +MockOutput->get_info, [
[' + ', 'roles'],
[' + ', 'users @alpha'],
[' + ', 'widgets @beta'],
+ [' - ', 'widgets @beta'],
[' - ', 'users @alpha'],
[' - ', 'roles'],
], 'Should have seen deploy and revert messages';
is_deeply +MockOutput->get_vent, [
['ROFL'],
- [__ 'Reverting all changes']
+ [__ 'Deploy failed' ],
+ [__ 'Reverting all changes'],
], 'The original error should have been vented';
$die = '';
@@ -921,16 +927,21 @@ DEPLOYDIE: {
@resolved = (0, '232213', '2352354');
throws_ok { $engine->deploy_change($change) } 'App::Sqitch::X',
'Shuld die on deploy failure';
- is $@->message, 'AAAH!', 'Should be the underlying error';
+ is $@->message, __ 'Deploy failed', 'Should be told the deploy failed';
is_deeply $engine->seen, [
[ change_id_for_depend => $conflicts[0] ],
[ change_id_for_depend => $requires[0] ],
[ change_id_for_depend => $requires[1] ],
[run_file => $change->deploy_file],
+ [run_file => $change->revert_file],
[log_fail_change => $change],
], 'It should failed to have been deployed';
+ is_deeply +MockOutput->get_vent, [
+ ['AAAH!'],
+ ], 'Should have vented the original error';
is_deeply +MockOutput->get_info, [
- [' + ', $change->format_name]
+ [' + ', $change->format_name],
+ [' - ', $change->format_name],
], 'Should have shown change name';
is_deeply [ map { $_->resolved_id } @conflicts ], [undef],
'Non-conflicting dependency should not have resolved';
View
9 t/x.t
@@ -64,4 +64,13 @@ try {
is $_->ident, 'io', 'Should be an "io" exception';
};
+# Make sure we can goto hurl.
+try {
+ @_ = (io => 'Cannot open file');
+ goto &hurl;
+} catch {
+ return fail "Not a Sqitch::X: $_" unless eval { $_->isa('App::Sqitch::X') };
+ is $_->ident, 'io', 'Should catch error called via &goto';
+};
+
done_testing;
Please sign in to comment.
Something went wrong with that request. Please try again.