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

RESOLVED FIXED in Bugzilla 5.0

Status

()

P1
enhancement
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: LpSolit, Assigned: mail)

Tracking

Bugzilla 5.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
Except that should be Bugzilla->request_cache, and not Bugzilla->params.
(Reporter)

Comment 2

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

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

Comment 3

10 years ago
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).
(Reporter)

Updated

9 years ago
Assignee: email-notifications → LpSolit
Priority: -- → P1
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6
(Reporter)

Updated

9 years ago
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8

Comment 4

8 years ago
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.
(Reporter)

Updated

8 years ago
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.
Created attachment 735060 [details] [diff] [review]
patch v1
Assignee: email-notifications → glob
Status: NEW → ASSIGNED
Attachment #735060 - Flags: review?(LpSolit)
(Reporter)

Updated

5 years ago
Target Milestone: --- → Bugzilla 5.0
Blocks: 181059
(Reporter)

Comment 7

5 years ago
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)

Updated

5 years ago
Assignee: glob → sgreen
(Assignee)

Comment 8

4 years ago
Created attachment 8396863 [details] [diff] [review]
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.
Attachment #8396863 - Flags: review?(glob)
(Assignee)

Updated

4 years ago
Attachment #8396863 - Flags: review?(glob) → review?(dkl)
(Assignee)

Updated

4 years ago
Attachment #8396863 - Flags: review?(dkl) → review?
(Assignee)

Comment 9

4 years ago
Created attachment 8401035 [details] [diff] [review]
v3 patch
Attachment #735060 - Attachment is obsolete: true
Attachment #8396863 - Attachment is obsolete: true
Attachment #8396863 - Flags: review?
Attachment #8401035 - Flags: review?(dkl)
(Assignee)

Comment 10

4 years ago
Created attachment 8406634 [details] [diff] [review]
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)
Attachment #8401035 - Attachment is obsolete: true
Attachment #8401035 - Flags: review?(dkl)
Attachment #8406634 - Flags: review?(justdave)
(Assignee)

Updated

4 years ago
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+

Updated

4 years ago
Flags: approval?
(Assignee)

Comment 12

4 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f4b9806..2863a66  master -> master
Flags: approval? → approval+
(Reporter)

Comment 13

4 years ago
I guess this bug can be closed as FIXED...
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Updated

3 years ago
Blocks: 1186700
(Reporter)

Updated

3 years ago
Blocks: 1194987
You need to log in before you can comment on or make changes to this bug.