448574 - Let $dbh->bz_commit_transaction send emails which are generated during a transaction

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement, P1)

Description

[Frédéric Buclin]

In some places, such as process_bug.cgi and editproducts.cgi (more specifically: Product::update), we need transactions to avoid race conditions and DB inconsistencies, but we also have emails to send, e.g. to users related to some bugs and/or to voters when product attributes are modified.

In Bugzilla::Foo::create() and ::update(), you have no idea if the caller started a transaction and so it's in some cases painful to decide if you should call MessageToMTA() directly or put emails in an array and let the caller send them when the transaction is over (to avoid sending emails despite a transaction failed). On the other hand, you also don't want a transaction to fail only because your MTA was unable to send emails (e.g. due to a malformed bugmail).

I discussed this with mkanat and we found a solution which looks very promising:
We call MessageToMTA when we need to, and MessageToMTA will check if a transaction is running or not:
- If no transaction is running, it sends emails directly as it does currently.
- If a transaction is running, it queues emails in e.g. Bugzilla->params->{mailer_transaction_queue} (or Bugzilla->params->{LpSolit_has_great_ideas} :-D ), and when $dbh->bz_commit_transaction is called, it checks if $dbh->{private_bz_transaction_count} is 0, in which case it can clear the email queue after $dbh->commit has been called. If the user interrupts the script before bz_commit_transaction is called, emails are lost, which doesn't matter as changes have not been committed to the DB.

This solution lets us start a transaction *before* the first call to $foo->set_bar() and terminate it *after* $foo->update(), excluding many race conditions.

Comment 1

[Max Kanat-Alexander]

Except that should be Bugzilla->request_cache, and not Bugzilla->params.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> Except that should be Bugzilla->request_cache, and not Bugzilla->params.

Argh... Of course, right. :-/

Comment 3

[Max Kanat-Alexander]

Also, if we don't have a $dbh->bz_in_transaction that returns a boolean, we should (instead of checking {private_bz_transaction_count} directly).

Comment 4

[Max Kanat-Alexander]

I think one possible solution to this might be to use TheSchwartz::Simple to always queue mails, and then to have custom code in bz_commit_transaction to pull them out of the database and send them if we're not in use_mailer_queue mode.

Comment 5

[:glob ✱]

(In reply to Max Kanat-Alexander from comment #4)
> I think one possible solution to this might be to use TheSchwartz::Simple to
> always queue mails, and then to have custom code in bz_commit_transaction to
> pull them out of the database and send them if we're not in use_mailer_queue
> mode.

that won't work because theschwartz begins a transaction when inserting jobs, and mysql doesn't support nested transactions.


i do prefer inserting email into a table instead of storing them in memory, as some installations generate a large number of emails with each bug update.  to do this we'd have to create a new table which temporarily holds composed bugmail, which is then inserted into theschwartz queues immediately after the transaction is committed.

Comment 6

[:glob ✱]

Attachment: patch v1

Comment 7

[Frédéric Buclin]

Comment on attachment 735060 [details] [diff] [review]
patch v1

>=== modified file 'Bugzilla/Mailer.pm'

>+    # email immedately, in case the transaction is rolled back.  Instead we

s/immedately/immediately/


>+    if ($dbh->bz_in_transaction()) {
>+        $dbh->do("INSERT INTO mail_staging(message) VALUES(?)", undef, $email->as_string);
>+        return;
>+    }

You should move this check before reaching the jobqueue code, else the email will be inserted into TheSchwartz tables anyway and be processed by it even if the transaction fails.


>+sub send_staged_mail {
>+    foreach my $row (@{ $dbh->selectall_arrayref("SELECT id,message FROM mail_staging") }) {

To make the code a bit cleaner, I would prefer something like:

    my $emails = $dbh->...
    foreach my $email (@$emails) {
        ...
    }


>+The actual behaviour depends on a number of factors: if called from within a
>+database transaction, the message will be staged and sent when the transaction
>+is committed.  If email queueing is enabled, the message will be sent to
>+TheSchwartz job queue where it will be processed by the jobqueue daemon.

... else the message is sent immediately.


I didn't test your patch yet, but it looks good.

Comment 8

[Simon Green]

Attachment: bug448574-v2.patch

> You should move this check before reaching the jobqueue code, else
> the email will be inserted into TheSchwartz tables anyway and be
> processed by it even if the transaction fails.

Since I still wanted to store the e-mail as plain text rather than an object, I didn't move the code. Rather I added a && ! $dbh->in_transaction to the code that determines whether to add e-mail to TheSchwartz.

This does mean that any e-mail that is sent while in a transaction has an extra to-string -> to_object conversion. The difference between that and storing an object (which has to be stringified anyway) is probably very negligible.

Comment 9

[Simon Green]

Attachment: v3 patch

Comment 10

[Simon Green]

Attachment: bug448574-v4.patch

This addresses a minor issue when the database isn't available. Red Hat have a customisation that e-mails an error message when db_new fails (mainly because we don't want to display connection details on the web page). The change in this patch means that even if the database is not available, a mail will always be sent if $send_now is sent (it checks this before checking dbh->in_transaction)

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8406634 [details] [diff] [review]
bug448574-v4.patch

Review of attachment 8406634 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine and works as expected. r=dkl

Comment 12

[Simon Green]

To ssh://[e-mail]/bugzilla/bugzilla.git
   f4b9806..2863a66  master -> master

Comment 13

[Frédéric Buclin]

I guess this bug can be closed as FIXED...