0017460: Email notifications are sent in batches - MantisBT - Signup temporarily disabled due to spam

ID	Project	Category	View Status	Date Submitted	Last Update

0017460	mantisbt	email	public	2014-06-19 12:41	2015-09-06 17:37

Reporter	dregad	Assigned To	dregad
Priority	normal	Severity	block	Reproducibility	N/A
Status	closed	Resolution	fixed
Product Version	1.3.0-beta.1
Target Version	1.3.0-beta.3	Fixed in Version	1.3.0-beta.3

Summary	0017460: 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
Tags	No tags attached.

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 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 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 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 2015-01-24 15:18 manager ~0048671	@dregad agreed with reverting the change over chat.

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 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 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 2015-01-26 04:00 developer ~0048688	reverts, [...] mixed with merges/conflicts Are always a pain... ;) I'll give it a shot.

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 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 2015-03-07 23:30 manager ~0049150	@dregad, what is the plan here? I marked this as blocking for 1.3.

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 2015-04-01 11:05 developer ~0049311	Pull request for review https://github.com/mantisbt/mantisbt/pull/589

MantisBT: master 9c45e146 2015-03-30 05:41 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	Affected Issues 0017458, 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 06:58 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	Affected Issues 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 02:34 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	Affected Issues 0017460
	mod - core/email_api.php	Diff File

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

View Issue Details

Relationships

Activities

Related Changesets