Closed Bug 448574 Opened 13 years ago Closed 7 years ago

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

Categories

(Bugzilla :: Email Notifications, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: LpSolit, Assigned: mail)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Except that should be Bugzilla->request_cache, and not Bugzilla->params.
(In reply to comment #1)
> Except that should be Bugzilla->request_cache, and not Bugzilla->params.

Argh... Of course, right. :-/
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).
Assignee: email-notifications → LpSolit
Priority: -- → P1
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8
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.
Assignee: LpSolit → email-notifications
Target Milestone: Bugzilla 4.0 → ---
(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.
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: email-notifications → glob
Status: NEW → ASSIGNED
Attachment #735060 - Flags: review?(LpSolit)
Target Milestone: --- → Bugzilla 5.0
Blocks: 181059
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.
Attachment #735060 - Flags: review?(LpSolit) → review-
Assignee: glob → sgreen
Attached patch bug448574-v2.patch (obsolete) — Splinter Review
> 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.
Attachment #8396863 - Flags: review?(glob)
Attachment #8396863 - Flags: review?(glob) → review?(dkl)
Attachment #8396863 - Flags: review?(dkl) → review?
Attached patch v3 patch (obsolete) — Splinter Review
Attachment #735060 - Attachment is obsolete: true
Attachment #8396863 - Attachment is obsolete: true
Attachment #8396863 - Flags: review?
Attachment #8401035 - Flags: review?(dkl)
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)
Attachment #8401035 - Attachment is obsolete: true
Attachment #8401035 - Flags: review?(dkl)
Attachment #8406634 - Flags: review?(justdave)
Attachment #8406634 - Flags: review?(justdave) → review?(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
Attachment #8406634 - Flags: review?(dkl) → review+
Flags: approval?
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f4b9806..2863a66  master -> master
Flags: approval? → approval+
I guess this bug can be closed as FIXED...
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 1186700
Blocks: 1194987
You need to log in before you can comment on or make changes to this bug.