From 56596e5240ef920df14d99087451ce6f06ac6d36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 11 Oct 2018 13:40:05 +0200 Subject: [PATCH] [FIX] mail: avoid concurrent update of notifications When sending notifications by batch one notification email can be send up to 50 people. For one mail_mail entry to handle 50 emails can be sent as those are sent independently for each recipient. In some cases a bounce may occur while the whole batch of recipients is not completely mailed. In that case the mailgateway will update the notification status to bounced. When the cron finishes to send the whole batch of emails it tries to update the notifications of all recipients. However as a notification has already been updated due to the bounce we face a concurrent update, meaning the transaction is rollbacked. Emails have been sent but notifications are not considered as sent as they have not been updated accordingly. Next time the cron runs it will send the same batch again, with probably the same bounce and rollback. We could therefore face an email loop. This commit fixes that by setting notifications to a transient exception state. Notifications are locked, meaning bounced will have to wait for the transaction to finish before updating them. That way the cron can run safely on the whole batch and we avoid having emails loop. It has an impact on query count as a search and write is performed in the send process. This commit is linked to task ID 1893054. Done in collaboration with @Xavier-Do . --- addons/mail/models/mail_mail.py | 17 ++++++++++++++++- addons/test_mail/tests/test_performance.py | 20 ++++++++++---------- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/addons/mail/models/mail_mail.py b/addons/mail/models/mail_mail.py index a4243a95c35..c02eab8989b 100644 --- a/addons/mail/models/mail_mail.py +++ b/addons/mail/models/mail_mail.py @@ -281,7 +281,6 @@ class MailMail(models.Model): values['partner_id'] = partner email_list.append(values) - # headers headers = {} ICP = self.env['ir.config_parameter'].sudo() @@ -305,6 +304,22 @@ class MailMail(models.Model): 'state': 'exception', 'failure_reason': _('Error without exception. Probably due do sending an email without computed recipients.'), }) + # Update notification in a transient exception state to avoid concurrent + # update in case an email bounces while sending all emails related to current + # mail record. + notifs = self.env['mail.notification'].search([ + ('is_email', '=', True), + ('mail_id', 'in', mail.ids), + ('email_status', 'not in', ('sent', 'canceled')) + ]) + if notifs: + notif_msg = _('Error without exception. Probably due do concurrent access update of notification records. Please see with an administrator.') + notifs.write({ + 'email_status': 'exception', + 'failure_type': 'UNKNOWN', + 'failure_reason': notif_msg, + }) + # build an RFC2822 email.message.Message object and send it without queuing res = None for email in email_list: diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index 8c109017789..d438d1f91b0 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -199,7 +199,7 @@ class TestAdvMailPerformance(TransactionCase): self.user_test.write({'notification_type': 'email'}) record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=65, emp=83): # com runbot: 64 - 82 // test_mail only: 65 - 83 + with self.assertQueryCount(margin=1, admin=67, emp=85): # com runbot: 67 - 85 // test_mail only: 67 - 85 record.write({ 'user_id': self.user_test.id, }) @@ -253,7 +253,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_one_email_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=61, emp=79): # com runbot: 54 - 72 // test_mail only: 61 - 79 + with self.assertQueryCount(margin=1, admin=63, emp=81): # com runbot: 56 - 74 // test_mail only: 63 - 81 record.message_post( body='

Test Post Performances with an email ping

', partner_ids=self.customer.ids, @@ -376,7 +376,7 @@ class TestHeavyMailPerformance(TransactionCase): }) mail_ids = mail.ids - with self.assertQueryCount(admin=13, emp=20): # test_mail only: 13 - 20 + with self.assertQueryCount(admin=14, emp=21): # test_mail only: 14 - 21 self.env['mail.mail'].browse(mail_ids).send() self.assertEqual(mail.body_html, '

Test

') @@ -389,7 +389,7 @@ class TestHeavyMailPerformance(TransactionCase): self.umbrella.message_subscribe(self.user_portal.partner_id.ids) record = self.umbrella.sudo(self.env.user) - with self.assertQueryCount(admin=97, emp=120): # com runbot 90 - 113 // test_mail only: 97 - 120 + with self.assertQueryCount(admin=101, emp=124): # com runbot 95 - 118 // test_mail only: 101 - 124 record.message_post( body='

Test Post Performances

', message_type='comment', @@ -406,7 +406,7 @@ class TestHeavyMailPerformance(TransactionCase): record = self.umbrella.sudo(self.env.user) template_id = self.env.ref('test_mail.mail_test_tpl').id - with self.assertQueryCount(admin=116, emp=151): # com runbot 109 - 144 // test_mail only: 116 - 151 + with self.assertQueryCount(admin=120, emp=155): # com runbot 114 - 149 // test_mail only: 120 - 155 record.message_post_with_template(template_id, message_type='comment', composition_mode='comment') self.assertEqual(record.message_ids[0].body, '

Adding stuff on %s

' % record.name) @@ -476,7 +476,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id) - with self.assertQueryCount(admin=67, emp=85): # com runbot: 66 - 84 // test_mail only: 67 - 85 + with self.assertQueryCount(admin=69, emp=87): # com runbot: 69 - 87 // test_mail only: 69 - 87 rec.write({'user_id': self.user_portal.id}) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) @@ -499,7 +499,7 @@ class TestHeavyMailPerformance(TransactionCase): customer_id = self.customer.id user_id = self.user_portal.id - with self.assertQueryCount(margin=1, admin=196, emp=232): # com runbot: 194 - 231 // test_mail only: 196 - 232 + with self.assertQueryCount(margin=1, admin=202, emp=238): # com runbot: 202 - 238 // test_mail only: 202 - 238 rec = self.env['mail.test.full'].create({ 'name': 'Test', 'umbrella_id': umbrella_id, @@ -528,7 +528,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(margin=1, admin=124, emp=143): # com runbot: 123 - 142 // test_mail only: 124 - 143 + with self.assertQueryCount(margin=1, admin=129, emp=148): # com runbot: 129 - 148 // test_mail only: 129 - 148 rec.write({ 'name': 'Test2', 'umbrella_id': self.umbrella.id, @@ -566,7 +566,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(margin=1, admin=130, emp=153): # test_mail only: 130 - 153 + with self.assertQueryCount(margin=1, admin=134, emp=157): # test_mail only: 134 - 157 rec.write({ 'name': 'Test2', 'umbrella_id': umbrella_id, @@ -600,7 +600,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) - with self.assertQueryCount(admin=55, emp=76): # test_mail only: 55 - 76 + with self.assertQueryCount(admin=56, emp=77): # test_mail only: 56 - 77 rec.write({ 'name': 'Test2', 'customer_id': customer_id,