View Issue Details

IDProjectCategoryView StatusLast Update
0017460mantisbtemailpublic2015-09-06 17:37
ReporterdregadAssigned Todregad 
PrioritynormalSeverityblockReproducibilityN/A
Status closedResolutionfixed 
Product Version1.3.0-beta.1 
Target Version1.3.0-beta.3Fixed in Version1.3.0-beta.3 
Summary0017460: Email notifications are sent in batches
Description

We need to process email notifications in a way that ensures they are systematically sent when not relying on a cron job.

Due to a regression introduced by [1], they were only sent from the GUI (in html_end()); the problem was fixed for SOAP in 0017458 but the issue remains for plugins (e.g. emailreporting [2]).

[1] https://github.com/mantisbt/mantisbt/commit/cea405ccf228fd2c6ac694574a74e87396b14f1f
[2] https://github.com/mantisbt/mantisbt/commit/cea405ccf228fd2c6ac694574a74e87396b14f1f#commitcomment-5192082

Additional Information

grangeway suggested to rely on register_shutdown_function() in [3]

Not sure the event_callback suggestion makes sense (as we want the event api to be fast as possible to check hooks etc) however We could add along the lines of the following to core.php:

if( php_sapi_name() == 'cli' ) { 
    register_shutdown_function(email_send_all()); 
}

Although, I'm still thinking it's probably a case that we need to provide recommendations for plugin authors on how to handle cron jobs. In terms of the above, we'd need to check what you can do, as at least, there used to be limitations in what you could do in shutdown functions - but i think that was if a fatal error had occurred in a script previously.

atrol and rombert +1'd the idea

[3] https://github.com/mantisbt/mantisbt/commit/cea405ccf228fd2c6ac694574a74e87396b14f1f#commitcomment-5207228

TagsNo tags attached.

Relationships

related to 0017458 closeddregad SOAP API does not send e-mails 
has duplicate 0020067 closedatrol email queued but not sent, for status changes 
related to 0012382 acknowledged Long emails aren't sent and make Mantis freeze 

Activities

vboctor

vboctor

2015-01-14 03:05

manager   ~0042141

A couple of comments here:

  • Based on [1] the shutdown function will be called as part of the request cycle and can output data. Hence, the optimization by calling fastcgi_finish_request() in the existing code is lost. Hence, the code that runs on html_end() for this purpose should move to the shutdown function.

  • I wonder if there are any concerns about running email_send_all() on some pages (e.g. installation pages).

[1] http://php.net/manual/en/function.register-shutdown-function.php

vboctor

vboctor

2015-01-14 11:47

manager   ~0042156

I've also noticed that change [1] referenced in the description is only sending emails on html_end() which is not called for all pages. This causes emails to be sent in batches when an action with html_end() is sent out. I don't have a full list of native actions that are missing html_end(), but it seems that deleting an issue which leverages bug_actiongroup.php is one of them.

So this issue is higher priority than just plugins sending emails.

vboctor

vboctor

2015-01-15 01:42

manager   ~0042161

I believe also issue status updates and bug updates currently don't trigger email sending. The emails will be sent after an issue is reported.

vboctor

vboctor

2015-01-24 13:26

manager   ~0048668

I suggest we revert this change from 1.3. I'm not sure there is a real need for it given the cronjob feature. But if there is, we can re-add it in 2.0 when the issues are addressed. I suggest doing this pre-beta2.

vboctor

vboctor

2015-01-24 15:18

manager   ~0048671

@dregad agreed with reverting the change over chat.

vboctor

vboctor

2015-01-24 15:35

manager   ~0048672

These seem to be commits that need to be reverted:

Date: Jan 7th, 2014
Author: Paul Richards
Commit: cea405ccf228fd2c6ac694574a74e87396b14f1f

Date: Jan 17th, 2014
Author: Paul Richards
Sign-off By: Damien Regad
Commit: d233a9da657a2331876cbed161576046dcc7f4a6

Date: June 19th, 2014
Author: Damien Regad
Commit: a644706452a45877f69f63f0d6584836e81baea4

dregad

dregad

2015-01-26 03:23

developer   ~0048686

Be careful with a644706452a45877f69f63f0d6584836e81baea4, it's a merge commit... (maybe f45212519ab27d7b15e330ff360b38a08d0e9198 needs to be reverted as well)

vboctor

vboctor

2015-01-26 03:31

manager   ~0048687

I tried to get this done, but I wasn't able to. I'm not very good with reverts, specially when mixed with merges/conflicts. Would you be able to help with this one?

dregad

dregad

2015-01-26 04:00

developer   ~0048688

reverts, [...] mixed with merges/conflicts

Are always a pain... ;)
I'll give it a shot.

dregad

dregad

2015-01-29 11:35

developer   ~0048734

I just had a look at this, and I'm wondering if instead of reverting, we should not attempt to implement what was suggested earlier instead, i.e. register a shutdown function in core.php.

My rationale is that if we revert, and decide to go for the shutdown function later on, we'll have to redo this all over again.

Thoughts ?

vboctor

vboctor

2015-01-29 13:26

manager   ~0048737

I noticed that too much calling of email_send_all() would slow the app specially if there is queued mail that is erroring out. So are we going to be doing work only when the page generated emails?

I just think the old model was simple enough, inline or background. But I can be convinced otherwise. Can you describe how the approach end to end will work?

vboctor

vboctor

2015-03-07 23:30

manager   ~0049150

@dregad, what is the plan here? I marked this as blocking for 1.3.

vboctor

vboctor

2015-03-23 22:57

manager   ~0049286

This also impacts scenarios like signup. Where users are blocked on getting the email to make progress.

I'm still in favor of reverting.

dregad

dregad

2015-04-01 11:05

developer   ~0049311

Pull request for review https://github.com/mantisbt/mantisbt/pull/589

Related Changesets

MantisBT: master 9c45e146

2015-03-30 09:41:40

dregad

Details Diff
Synchronous email sending via shutdown function

Mantis provides 2 ways of processing email: synchronously (default) or
using a cron job. Historically, the former was achieved by processing
the email queue immediately after each email-generating action.

This approach could lead to a severe performance degradation when the
queue contains a backlog of pending messages which can't be sent (e.g.
due to invalid email addresses or server problems).

In early 2014, an attempt was made to improve this for fastcgi by
processing the queue at the bottom of each displayed page (in html_end()
function), which introduced regressions for SOAP API (issue 0017458) as
well as plugins and other corner cases (issues 0017460).

This commit resolves the problem by registering a shutdown function to
process the email queue in core.php, which ensures that email gets sent
no matter what.

To avoid multiple executions of the shutdown function for a single user
request (which may lead to several executions of core.php, e.g. when
building dynamic css, javascript translations, etc), an arbitrary 5
seconds delay is observed between each register_shutdown_function()
call.

Fixes 0017460
mod - api/soap/mantisconnect.php Diff File
mod - core.php Diff File
mod - core/email_api.php Diff File
mod - core/html_api.php Diff File

MantisBT: master ca7d087a

2015-04-04 10:58:53

dregad

Details Diff
Refactor the email shutdown function

Register email_shutdown_function() from a new generic helper API,
mantis_shutdown_functions_register(); this could be used to add other
shutdown functions or a plugin event in the future.

The shutdown function is now always registered, and the logic to
determine whether to call email_send_all() or not has been moved into
it. This allows removal of the delay implemented in the previous
version to avoid concurrent executions.

The $g_email_stored global variable has been renamed and is now used as
a binary flag which drives the shutdown function's behavior. By setting
this flag, the request can
- indicate that emails have been generated, allowing the shutdown
function to trigger email_send_all() only when necessary
- force sending of emails in the shutdown function, regardless of
$g_email_send_using_cronjob setting (useful e.g. for signup or
password reset processes, where we don't want to delay notifications).

To implement that behavior, a new optional parameter ($p_force) was
added to the email_store() function.

These changes are derived from vboctor's comments in PR 589.

Issue 0017460
mod - core.php Diff File
mod - core/constant_inc.php Diff File
mod - core/email_api.php Diff File
mod - core/helper_api.php Diff File
mod - core/logging_api.php Diff File

MantisBT: master 037b99a5

2015-04-05 06:34:42

dregad

Details Diff
email_store() should always set EMAIL_SHUTDOWN_GENERATED flag

Even though the code in email_shutdown_function() handles the
EMAIL_SHUTDOWN_FORCE flag as though EMAIL_SHUTDOWN_GENERATED were also
set, it is technically more correct to always set the flag.

Issue 0017460
mod - core/email_api.php Diff File

Issue History

Date Modified Username Field Change
2014-06-19 12:41 dregad New Issue
2014-06-19 12:41 dregad Relationship added related to 0017458
2014-12-13 17:18 vboctor Product Version 1.3.0dev => 1.3.0-beta.1
2015-01-14 03:05 vboctor Note Added: 0042141
2015-01-14 11:47 vboctor Note Added: 0042156
2015-01-15 01:42 vboctor Note Added: 0042161
2015-01-15 01:58 vboctor Status new => confirmed
2015-01-15 01:58 vboctor Summary Implement a global and generic way of sending e-mail notifications => Email notifications are sent in batches
2015-01-15 01:58 vboctor Target Version => 1.3.0-beta.2
2015-01-24 13:26 vboctor Note Added: 0048668
2015-01-24 15:18 vboctor Note Added: 0048671
2015-01-24 15:35 vboctor Note Added: 0048672
2015-01-26 03:23 dregad Note Added: 0048686
2015-01-26 03:31 vboctor Note Added: 0048687
2015-01-26 04:00 dregad Note Added: 0048688
2015-01-29 11:35 dregad Note Added: 0048734
2015-01-29 13:26 vboctor Note Added: 0048737
2015-03-07 23:29 vboctor Severity major => block
2015-03-07 23:30 vboctor Note Added: 0049150
2015-03-15 19:59 dregad Target Version 1.3.0-beta.2 => 1.3.0-beta.3
2015-03-23 22:57 vboctor Note Added: 0049286
2015-04-01 11:05 dregad Note Added: 0049311
2015-04-05 19:41 dregad Changeset attached => MantisBT master 9c45e146
2015-04-05 19:41 dregad Changeset attached => MantisBT master ca7d087a
2015-04-05 19:41 dregad Changeset attached => MantisBT master 037b99a5
2015-04-05 19:41 dregad Assigned To => dregad
2015-04-05 19:41 dregad Status confirmed => resolved
2015-04-05 19:41 dregad Resolution open => fixed
2015-04-05 19:41 dregad Fixed in Version => 1.3.0-beta.3
2015-04-19 07:35 atrol Relationship added related to 0012382
2015-09-01 13:09 atrol Relationship added has duplicate 0020067
2015-09-06 17:37 vboctoradmin Status resolved => closed