About the implementation:

• This sample code doesn't actually work, does it? The use transactions part would be executed at compile time before $dbh has a value.
• What happens in long running environments like mod_perl when $dbh changes to a new database handle (because of timeout and reconnect with Apache::DBI)? If you only pass in $dbh at compile time (because it's done with a use), the $dbh inside transactions will never get updated.
• Defining your subs inline like that looks pretty strange.
• I don't think you need to take a reference to $_[1], since it is a ref already.
• Creating a routine that takes sub refs but does not appear to (like transaction()) leads to many scoping bugs. People will not understand why return $foo inside of a transaction block doesn't actually return. They will create closures that leak memory by accident. (See the info about Error.pm on this site and elsewhere.) I suggest using a source filter instead. Look at Filter::Simple to see how it can be done.

Well, personally, I would just put it under DBIx::Transaction. It's just a syntactic sugar module, and doesn't actually implement any kind of transactions. Calling it Transactions could be misleading to people.

I'm open to suggestions, but not to DBIx::. This module has a much wider scope. Currently, I know of no other modules that use begin_work, rollback and commit, but in theory any transaction capable module could be used with this.

This sample code doesn't actually work, does it? The use transactions part would be executed at compile time before $dbh has a value.

That's correct. I tested without a database, and was lazy so I supplied a string literal. I thought it was possible to reference the container instead of the value, but I was wrong. No, the module doesn't work.

If you only pass in $dbh at compile time (because it's done with a use), the $dbh inside transactions will never get updated.

That will have to be changed.

Defining your subs inline like that looks pretty strange.

That's because they're closures that share the same $dbh. Kind of useless if you consider that $dbh is a reference to undef, but I didn't know that at that time.

I don't think you need to take a reference to $_1, since it is a ref already.

That was because it was used in other subs, which have @_'s of their own. Needed to reference to avoid copying.

Creating a routine that takes sub refs but does not appear to (like transaction()) leads to many scoping bugs.

I will document that. Sourcefilters are an ugly solution. I'd rather introduce one very obvious bug than lots of subtle parsing bugs.

Maybe I'll make the module OO after all, but I think this looks ugly:

my $t = Transaction->new($dbh);
$t->transaction(sub {
    for (1..10) {
        my $sth = $dbh->prepare(...);
        $sth->execute() or $t->rollback;
    }
});
[download]

Or I can just have a function called transaction_select (lots of typing - better names welcome)

transaction_select $dbh;
transaction {
    for (1..10) {
        my $sth = $dbh->prepare(...);
        $sth->execute() or rollback;
    }
};
[download]

One thing's for sure: the module as pasted in this thread's root node does not work and needs to be fixed. Thanks for pointing that out.

Update: new version, with working code. I thought the OO solution was way too ugly and I didn't like transaction_select either. I decided to use a package global $T instead.

Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

I'm open to suggestions, but not to DBIx::. This module has a much wider scope. Currently, I know of no other modules that use begin_work, rollback and commit, but in theory any transaction capable module could be used with this.

...then don't tie people in to the method names, provide them with the ability to redefine them. a caller might do:

use My::Transactions 0.01 
  begin_work => try_it,
  rollback => fallback,
  commit => do_it;
[download]

~Particle *accelerates*

But, the module czars will tell you that.

-- Randal L. Schwartz, Perl hacker
Be sure to read my standard disclaimer if this is a reply.

Even though it doesn't have DBIx in the name, it still cannot begin with a lowercase letter.

Very good point.

It probably shouldn't be a top-level name anyway.

I couldn't find an existing namespace to fit it in.

Currently, I think Transactions is a good name. If anyone has a better name, let me know. Please keep it short, because "use Transactions" can be used as a sort of select if you use multiple transaction capable objects.

Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

It "cannot" begin with a lowercase letter? Why is that?

Abigail

Those are reserved for pragmata. If you're not a P5P patch generating person, you don't get to use lowercase. {grin}

-- Randal L. Schwartz, Perl hacker
Be sure to read my standard disclaimer if this is a reply.

+-------------+-------------+------+-----+---------+-------+
| Field       | Type        | Null | Key | Default | Extra |
+-------------+-------------+------+-----+---------+-------+
| id          | int(11)     | YES  |     | NULL    |       |
| name        | varchar(64) | YES  |     | NULL    |       |
| description | varchar(64) | YES  |     | NULL    |       |
+-------------+-------------+------+-----+---------+-------+

use strict;
use warnings;
use DBI;

my $dbh = DBI->connect( ... );
use transactions $dbh;

$dbh->do('delete from foo');

transaction {
   for (1..10) {
      my $sth = $dbh->prepare('insert into foo values(?,?,?)');
      $sth->execute($_, chr(ord('a')+$_), chr(ord('z')-$_));
      rollback if $_ == 5;
   }
};

print map "@$_\n", @{$dbh->selectall_arrayref('select * from foo')};
$dbh->disconnect;

__END__
results:
1 b y
2 c x
3 d w
4 e v
5 f u
[download]

jeffa

L-LL-L--L-LL-L--L-LL-L--
-R--R-RR-R--R-RR-R--R-RR
B--B--B--B--B--B--B--B--
H---H---H---H---H---H---
(the triplet paradiddle with high-hat)

Are you using a BerkeleyDB or InnoDB or Gemini table type? The default MyISAM table type didn't support transactions for most of MySQL's life. (I'm not sure if the latest versions do.)

I honestly expected there to be no output ... shouldn't a rollback cause all 'inserted' data to be 'undone'?

It seems that your MySQL (table) does not support transactions, or that my module is broken. I haven't been able to try it with a real DB *yet*, and have only used print to see what methods it executes, but everything seems to be in order. Could you please check if transactions work if you use them without this module?

The module does not work the way I expected it to work. I should have tested it with a database.

Update: New version. This one actually works. I have used your code to test it:

use warnings;
use Transactions;
use DBI;

my $dbh = DBI->connect("dbi:SQLite:db.dat");

our $T = $dbh;

$dbh->do('delete from foo');

transaction {
    for (1..10) {
        my $sth = $dbh->prepare('insert into foo values(?,?,?)');
        $sth->execute($_, chr(ord('a')+$_), chr(ord('z')-$_));
        rollback if $_ == 5;
    }
};

print map "@$_\n", @{$dbh->selectall_arrayref('select * from foo')};
$dbh->disconnect;
[download]

Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

What might be interesting is to extend this to other actions as well -- creating/updating/deleting/renaming files, sending emails, etc. As you say, anything that can receive the 'begin_work', 'commit' and 'rollback' messages can participate. Making the messages observations (using something like, oh, Class::Observable) would be very flexible as well and you'd get for free object/class/subroutine registration to receive the messages. Just a thought.

I've found the Unit Of Work pattern most helpful with this. Martin Fowler used to have a lengthy description of it on his site but he went and published a book and changed the description to a simple summary. (The book is highly recommended, BTW.)

Chris
M-x auto-bs-mode

Making the messages observations (using something like, oh, Class::Observable) would be very flexible as well and you'd get for free object/class/subroutine registration to receive the messages. Just a thought.

I don't understand its documentation, and think the module is too advanced. Perhaps it's one of those things for which you have to know Java in order to understand. I made transactions.pm to be tiny and easy, and using a heavy module would not help it be easy to use.

Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

Sorry if I wasn't clear -- I wasn't suggesting that what you've proposed would be made better using the module. Just that if you ever decided to expand it (which often happens IME) it might be useful to look into further decoupling the transactionable (is that a word?) resources from the action doing the commit/rollback.

I don't think the Observer/Observable pattern is too advanced for common use. In fact I think it's one of the simplest (and quickest) ways you can achieve decoupling among disparate processes. Maybe the documentation for Class::Observable is a little too long, but that often happens with simple, flexible little tools that can be used in lots of different ways.

Good luck!

Chris
M-x auto-bs-mode

Nice idea. I don't like the global $T though. Not only you have to do

our $T = $object;
transaction {...}
[download]

transaction [$dbh1, $dbh2] => code{...};
# or
transaction ($dbh1, $dbh2) => code{...};
[download]

package Transactions;
use strict;
use Carp qw(croak);
use Exporter::Tidy default => [ qw(transaction commit rollback code) ]
+;

our $VERSION = '0.04j';
my @trans;
my $trancount = 0;

sub commit () {
    my $vars = pop @trans or croak "commit called outside transaction!
+";
    $trancount--;
    if ($trancount == 0) { # the outermost transaction
        $_->commit for reverse @$vars;
    } else {
        push @{$trans[-1]}, @$vars;
    }
    local $^W;  # "exiting sub via last"
    last __TRANSACTION;
}

sub rollback () {
    my $vars = pop @trans or croak "rollback called outside transactio
+n!";
    $trancount--;
    $_->rollback for reverse @$vars;
    local $^W;  # "exiting sub via last"
    last __TRANSACTION;
}

sub transaction ($$;@) {
    my $block = pop @_;
    my @vars = @_;
    @vars = @{$vars[0]} if (@vars == 1 and ref($vars[0]) eq 'ARRAY');
    push @trans, \@vars;
    $trancount++;
    __TRANSACTION: {
        eval {
            $_->begin_work for @vars;
            $block->();
        };
        $@ ? rollback : commit;
    }
    $@ and croak $@ . "commit not safe after errors, transaction rolle
+d back";
}
sub code (&) {return $_[0]}

1;
[download]

Another small fix is that the "commit not safe after errors, transaction rolled back" message should be croak()ed, not die()d.

Jenda
Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.
   -- Rick Osborne

Edit by castaway: Closed small tag in signature

but it also means that there may only be one object in the transaction.

See my example in another node in this thread about how you can easily write a wrapper package to allow this.

sub code (&) {return $_[0]}

Why not just use sub instead of code and call like transaction [ $foo, $bar ], sub { ... };?

Another small fix is that the "commit not safe after errors, transaction rolled back" message should be croak()ed, not die()d.

Not really. Note that $@ already contains the caller's filename and line number. I think this looks very nice:

died at foo.pl line 13.
commit not safe after errors, transaction rolled back at Transactions.
+pm line 123.
[download]

I wanted a (&) prototype to allow the sub keyword to be left out. So I tried several solutions to select a transaction capable object, $T being my most recent try.

But the more I think about it, typing sub isn't so bad. As you say, if you simply supply the dbh to transaction(), nested transactions can be supported.

I'll whip up a third version soon.

Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

1) Because

transaction $bdh => code{ ... }
[download]

reads better than

transaction $bdh, sub { ... }
[download]

. That's the only reason.

2) Well, it may contain the filename&linenumber of the original error message, but not of the place where the transaction ends and where therefore it's commited or rolled back. I think it makes more sense to append the position of the closing brace of the transaction block than some line within Transaction.pm. Try to throw an exception within a second level transaction. You'll end up with something like:

Some error
commit not safe after errors, transaction rolled back at Transactions.
+pm line 123
commit not safe after errors, transaction rolled back at Transactions.
+pm line 123
[download]

Not very helpfull I'd think :-)

Here is the script I used to test my version of the module:

Read more... (1474 Bytes)

Jenda
Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.
   -- Rick Osborne

Edit by castaway: Closed small tag in signature

=head1 SYNOPSIS

    use Transactions;
    
    my $dbh = DBI->connect(...);
    our $T = $dbh;
[download]

Instead of using a global variable in the caller's package, I wonder if it mightn't be better to pass a reference to the variable to be used into the module when it's use'ed, like so:

package Transactions;

our $Object;

sub import {
  my $class = shift;
  $Object   = shift;

  # somehow fall back to Exporter::Tidy?
  # goto &Exporter::Tidy::import; maybe?
}

...

sub transaction(&) {
  ...
  eval {
    $$Object->begin_work();  # double $ because it's a ref-to-a-ref
    $block->();
  }
  ...
}

=head1 SYNOPSIS

    my $dbh = DBI->connect(...);

    use Transactions \$dbh;
    
    transaction {
        for (1..10) {
            my $sth = $dbh->prepare(...);
            $sth->execute() or rollback;
        }
    };

...
[download]

Just a thought. :) Of course, it makes it a little bit harder to integrate with Exporter::Tidy. *shrug*

bbfu
Black flowers blossom
Fearless on my breath

Instead of using a global variable in the caller's package, I wonder if it mightn't be better to pass a reference to the variable to be used into the module when it's use'ed

Funny that you mention it, as that is exactly what I did before. See perrin's reply to learn why that doesn't work.

Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

You'll have to forgive me, I didn't see the original code before it was changed. However, the following simple test works fine for me. It even handles the case where you change the value of the handle after use'ing the module. The key, of course, being that you have to explicitly take the reference to the variable in the use line (and never dereference it in the module until you're actually using it).

Test Module:

package MyTst;
use warnings;
use strict;

our $Object;

sub import {
  my $class = shift;
  $Object   = shift;

  {
    no strict 'refs';
    *{caller().'::transaction'} = \&transaction;
  }
}

sub transaction(&) {
  $$Object->begin();
  shift()->();
  $$Object->end();
}

1;
[download]

Test program:

#!/usr/bin/perl
use warnings;
use strict;

our $tst = TstPkgA->new();

use MyTst \$tst;

transaction {
  print "Transaction A!\n";
};

$tst = TstPkgB->new();

transaction {
  print "Transaction B!\n";
};

exit;



#***************  Test Classes  ***************#

package TstPkgA;

sub new {
  return bless {}, 'TstPkgA';
}

sub begin {
  print "TstPkgA::begin\n";
}

sub end {
  print "TstPkgA::end\n\n";
}

package TstPkgB;

sub new {
  return bless {}, 'TstPkgB';
}

sub begin {
  print "TstPkgB::begin\n";
}

sub end {
  print "TstPkgB::end\n";
}
[download]

Output:

TstPkgA::begin
Transaction A!
TstPkgA::end

TstPkgB::begin
Transaction B!
TstPkgB::end
[download]

Of course, none of this helps with the case where you want multiple packages to be able to use their own transaction handle. *shrug*

Update: I vote for crenz's way of handling this. You could do it (even the for-style) with source filters. I'm not sure I agree that filters are really a bad way to go with this, as you are adding syntax to the language. *shrug* As long as you handle it with care, you shouldn't necessarily add any parsing bugs. Update2: Heck, you could even steal code from TheDamian's Switch and blame any bugs on him. ;)

bbfu
Black flowers blossom
Fearless on my breath

use Transaction \$dbh;
[download]

use Transaction qw($dbh);
[download]

Makeshifts last the longest.

Why don't you just export $T?

That wouldn't allow different packages to have their own $Ts.

Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

If the goal is a module that can be used in multiple instances though then a package global is a horrible interface to begin with. How about something like this?

use Exporter::Tidy default => [ qw(transaction select_handle commit ro
+llback) ];

sub select_handle {
    require Carp;
    Carp::croak("Can't select handle outside transaction");
}

sub _cant_change {
    require Carp;
    Carp::croak("Can't change handle during transaction");
}

sub transaction (&) {
    my ($block) = @_;
    my ($caller, $handle);

    my $caller = caller();

    local *{"${caller}::select_handle"} = sub {
        $handle = shift;
        *{"${caller}::select_handle"} = \&_cant_change;
    };

    # ...
}
[download]

To use it you then have to say

transaction {
    select_handle($dhb);
    # ...
};
[download]

which allows to use the module for as many handles even within the same package as you like. You can even nest transactions with no adverse effects.

Makeshifts last the longest.

Personally, I don't like the package global. I'd prefer something like

    my $dbh = DBI->connect(...);

    transaction $dbh, {
        for (1..10) {
            my $sth = $dbh->prepare(...);
            $sth->execute() or rollback;
        }
    };
[download]

or even

    transaction($dbh) {
        # code
    };
[download]

to make it look like a for block. I have no idea how to make that work, though.

Also, an additional idea would be to catch die's in the handler and make them execute rollback (which would make your transactions a bit like exceptions).

I'd prefer something like transaction $dbh, {

I would prefer that too! But Perl 5's prototypes don't allow that syntax. It requires the "sub" keyword then. I'm not sure if I want that.

Also, an additional idea would be to catch die's in the handler and make them execute rollback.

Already does that, and has always done that :)

Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

sub commit () {
    ($T || ${ caller() . '::T' })->commit;
    goto END_XACTION;
}

sub rollback () {
    ($T || ${ caller() . '::T' })->rollback;
    goto END_XACTION;
}

sub transaction (&) {
    my ($block) = @_;
    local $T = ${ caller() . '::T' } or croak "\$T not set";

    local $@;
    eval {
        $T->begin_work;
        $block->();
    };
    $@ ? rollback : commit;

    END_XACTION: die $@ . "commit not safe after errors, transaction r
+olled back"
        if $@;
}
[download]

sub commit {
    require Carp;
    Carp::croak("Can't commit outside a transaction");
}

sub rollback {
    require Carp;
    Carp::croak("Can't rollback outside a transaction");
}


sub transaction (&) {
    my ($block) = @_;

    my $caller = caller();

    local *{"${caller}::commit"} = sub { die "!COMMIT\n" };
    local *{"${caller}::rollback"} = sub { die "!ROLLBACK\n" };

    local $@;
    eval {
        $T->begin_work;
        $block->();
    };

    if(!$@ or $@ eq "!COMMIT\n") {
        ${"${caller}::T"}->commit;
    }
    elsif($@ eq "!ROLLBACK\n") {
        ${"${caller}::T"}->rollback;
    }
    else {
        ${"${caller}::T"}->rollback;

        require Carp;
        Carp::croak($@ . "commit not safe after errors, transaction ro
+lled back");
    }
}
[download]

Makeshifts last the longest.

I would suggest you work with die instead though - that will work with nested transactions and require fewer hoop jumps. You can then also easily add some more error checking:

I don't like your goto LABEL idea at all, but I do like the idea of using die very much. It even makes sense! :)

Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

last is nothing but a glorified goto - esp the way your were using it.

Makeshifts last the longest.