| Project: | Notify |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
There are some problems with the Notify module on large/slow sites.
The first problem is that Notify uses time() all over the code, expecting it to return the same value each time. That's not how time() works :-) The end result is that Notify may include certain nodes more than once, or may miss certain nodes.
The second problem is that Notify will try to re-run at each call to cron(), unless it manages to get to the end. It seems to want to re-run from the start of the list.
If sites limit the total runtime of PHP scripts, or the total runtime of cron jobs, then Notify will never make "real" progress, and (worse) will spam users that are early in the table with many multiple copies of everything, whereas later users will get nothing.
I'm proposing this patch to alleviate problem 1. To solve problem 2, it's harder. Cron really should keep track of which user has gotten a "try" for each period, and try to send to the first N (say, 100 by default) users each time cron is invoked within the send period, until all users have been sent. This will allow Drupal to finish, and commit to the database, even on sites with limited runtimes. I am considering a patch, but I'm not versed enough in drupal db core stuff to be sure I do it right, so any feedback on how to solve 2 is welcome!
| Attachment | Size |
|---|---|
| notify_patch.txt | 3.29 KB |
Comments
#1
I'm also having problems with the same email being sent multiple times to some of the users because cron fails to complete the list of recipients. Although I'm not sure why, it has gotten worse lately: I went from sporadic cron fails and 1 or 2 repeated emails to having all cron jobs failed and only 3 or 4 email successfully sent (and "perpetually" repeated) per cron job. Might there be some kind of cumulative effect causing this issue?
I also came across this Job Queue project that seem to be a great candidate to solve this problem. Am I correct? Did anyone experiment with it?
Thank you.
#2
Well, I just did: simply replacing the call to "drupal_mail" following the example from Job Queue module works and, I believe, should solve the 2nd problem reported by jwatte.
In my case, it also showed that my problem is not within Notify but it's the smtp module that's breaking the cron job (will try an smtp issue). Still, with Job Queue and a "high frequency" cron all mails get dispatched.
Hope it helps.
#3
A better look at the emails sent showed me that the notify + job_queue option previously referred ends up with wrong usernames and uid in the emails. In order to have it right, some more tweaks are necessary on "notify_mail" function.
#4
This issue is a priority for me, but I'm going to be swamped for the next couple months on some large projects. I'd be thrilled to bring on a co-maintainer to work on this issue.
#5
Not sure anymore if Job Queue is the right solution, but a solution for scalability is definitely needed.
#6
Here is a patch I made that still needs to be tested thoroughly but appears to work. It's currently in use on http://www.redrocker.com/
The approach is to add a new column to the notify table so each user has its own send_last value. As emails are sent to each user, their send_last column is updated. When cron aborts from taking to long to complete, the notify_send_last system variable never gets set, so the next cron run it starts over, but now it compared each user's send_last value and skips any that are newer than the system value (which didn't get updated on the last run). I could have made it just compile updates since the user's send_last run, but decided that if cron is having trouble finishing in one run, it's better to let it get through all of the users first, and then the next run it will catch those.
#7
@yamboy Attempt to apply patch in #6 on latest 6.x-1.x-dev fails.
I have not yet looked at applying it manually.
Just testing out the module and think this may be needed when enabling the module on an existing site.
I have several questions that I am working my way through, but not specific to this patch.
#8
@yamboy Patch in #6 was applied manually to the latest 6.x-1.x-dev release and with simple testing seems to work fine.
The patch removes the check box on the admin/settings/notify for a notify check box on registration. I am still not clear on the way the notify module is designed to work on registering a new account and that is the subject of a separate issue.
Minor issue, on the user/xxx/notify at the bottom the last send date is shown followed by the "Save settings" button. I think the Save settings button should be on the next line by itself.
I have not changed the status to RTBC because I do not have a test setup with high volume to test the real purpose of this patch but it certainly does not seem to cause any problems in normal use.
Thanks.
#9
I think #6 is not an adequate solution; it' still better to use job queue or something in that vein to eliminate the issue of cron timeouts entirely. For Drupal 7, the Queue API and hook_cron_queue_info would be the obvious solution, but I'm not sure for Drupal 6. Patches implementing job_queue module would be seriously considered, however.
#10
@matt2000, while a proper solution is developed, the patch might be a workable temporary "hack" for those who need it.
I found that the patch in #6 also needs a slight correction to restore the fix in http://drupal.org/node/364585#comment-2539694 that was committed.
#11
I have been digging through the issues to further my understanding of the Notify module.
The patches in this issue thread actually seem to include several different things some of which are already included in the current release.
Just on the question of "need to queue mailings across multiple cron runs" there are suggestions of two approaches.
Comments #2, #3, and #5 discuss using job queue.
The other alternative by yamboy adds a column to the table which shows last send time on the admin display which can be useful.
I have stripped the yamboy patch down to only the parts related to this specific feature. Unfortunately I have not been able to make it into a single patch but rather two. They are attached for anyone interested.
I also find myself leaning towards this approach to avoid the added dependency on another module.
#12
We'll probably need a per-user timestamp to implement a queue anyway, so I'm OK with committing something like this before that point.
But I'm still concerned that we need to implement something to prevent a cron timeout in the first place. For example, what happens if a cron times out, and then new content is created before the next cron run? If i'm not mistaken, I think users would never be notified of that intervening content. This could be a big problem for sites that only send notifications once a day, for example.
Also, a few nit picks:
- The update hook should be named notify_update_6001().
- To roll a complete patch, checkout the module using CVS, then use `cvs diff -up . > name.patch` to generate your patch file.
- The commented out watchdog debugging could should be removed, or else added to a conditional block that runs based on an administrative setting.
#13
Maybe this is too old to worry about it, but anyway i have tried an aproach using Job Queue with the Limit the number of jobs per cron runpatch so server mail per hour limit is not reached.
#3 issue is solved by passing new parameters to drupal_mail.
It seems to be working on http://www.seresromanticos.com
I can't do a patch so here it is the hole module with modifications:
#14
applied the patch. working like charming.
please commit to the latest -dev.
#15
@parasolx which patch did you apply?
#16
#11 patch.. so far no problem regarding this.
but need to flush email queue since no email is send even cron was run
#17
i rebuild the yamboy patch #6, since it using the older version.
this patch make by used the latest -dev version
#18
kindly to test this patch with -dev version.
#19
@parasolx The patch in #18 does apply successfully to the dev release using -p1 however the patch is based on the one in comment #6 which includes several features some of which are addressed in other issues and that is what led me to stripping down that patch into those in comment #11.
Would you consider working from the patches in comment #11 with the comments in #12 or can you explain why you are favoring return to the patch in #6 ?
Thanks.
#20
@izmeez i'm testing by using Job queue module for that patch. so far it work and being queue but since this module focus on simple method. so i think dependency on other module would not so great.
re built new patch from comment #11. addon '' for Last send at user notify page. so it would stand as it is. kindly to test.
#21
Or used this patch if you need to make "Last send" text to be break from submit button.
suggested to apply patch #20 rather than this. this patch currently used at my site only not for real commit.
#22
running patch comment #21 on my real site with 3000+ users without any problem so far.
cron has been set to run every 3 days and it run perfectly until all users get notification email.
hope get more feedback from community, if there is no problem kindly to commit into -dev
#23
patch #21 solve this problem and work as it.
#24
Automatically closed -- issue fixed for 2 weeks with no activity.
#25
Thanks for the patch in #21. It applies and appears to work as expected.
In #12 the maintainer suggested the watchdog debugging be removed or made into a conditional. I am comfortable in it being present in the patch as a comment for a reminder and tool until such time as a conditional for administrative use is added.
I have changed the status to RTBC as it is up to the maintainer to determine when the status should be changed to fixed if this is committed.
P.S. With over 4600 sites reported as using this module I thought there would be more interest in seeing this patch committed. What solutions are others using or do they not have a problem?
#26
@parasolx I am changing the status to needs work.
The patch in comment #21 applies without difficulty, but there is something different between it and the two patches in comment #11 that causes applying the patch http://drupal.org/files/issues/870812-notify_defaults_and_details_permis... to fail.
I haven't determined what the difference is. Any thoughts?
Thanks.
#27
ok, the diff between the patches in comment #11 and that in #21 is as follows:
@@ -308,12 +308,12 @@ function notify_user_settings_form(&$for
'#options' => array(t('Disabled'), t('Enabled')),
'#description' => t('Include new comments in the notification mail.'),
);
- $form['uid'] = array('#type' => 'value', '#value' => $account->uid);
$form['notify_send_last'] = array('#type' => 'markup',
'#title' => t('Notify last send'),
'#description' => t('The date of the last notification for this user.'),
- '#value' => t('Last Send: !date', array('!date' => $notify->send_last > 0 ? date('n/j/y g:i a', $notify->send_last) : "never"))
- );
+ '#value' => '<div>' . t('Last Send: !date', array('!date' => $notify->send_last > 0 ? date('n/j/y g:i a', $notify->send_last) : "never")) . '</div>',
+ );
+ $form['uid'] = array('#type' => 'value', '#value' => $account->uid);
$form['submit'] = array('#type' => 'submit', '#value' => t('Save settings'));
return $form;
If this is indeed the preferred change then the patch in the other queue will need to be reworked.
#28
There is no other place in the notify module that "div" tags are included so they probably should not be added in this patch.
I also do not see any particular reason for moving the line:
$form['uid'] = array('#type' => 'value', '#value' => $account->uid);What do others say?
#29
@izmeez,
i add so that the text for last send locate at top of button. it is not necessary. you can remove it and rebuilt the patch to make it work out with http://drupal.org/files/issues/870812-notify_defaults_and_details_permis....
#30
Ok so I'm jumping into this issue cold it was closed and then re-opened. Is the patch in comment #21 solid and can it be committed or is there work still being done? Please comment.
#31
@ishmael-sanchez This issue has been in the works for more than two and a half years. It was closed automatically after being marked as fixed by @parasolx after contributing a patch.
It basically revolves around adding a last sent field to queue mailings across multiple cron runs. This is summarized in comments #11 and 12. The patch in #21 essentially builds on this and also adds a div as identified in comments #28 and #29. Not sure what the consensus is on adding a div here.
#32
Ok thanks @izmeez. I will let the others involved in this thread respond over the next couple days and unless there is a huge objection, I will commit the patch in #21.
#33
@izmeez: i think it's better to commit patch at comment #20 rather than #21. patch #21 is done from which issues i was read before state that the phrase "Last send" should be above button which i comes to add .
already edit my recent post for comment #21 to avoid confusion. both patch at #20 and #21 can be used but since a lot of patch in other issues were related, I suggest to commit patch #20.
@ishmael-sanchez: i suggest to commit patch #20.
#34
@parasolx Looking at the patches in #20 and #21 they both look very similar and they both include the introduction of div. Since this module does not have any css files and this is the only place in the entire module that a div is introduced, is there really a need to add this div or is it a separate issue that is being rolled in? I am trying to understand why this is being added here. Maybe a screen capture might help to understand.
#35
Agreed, the div needs to be removed before this can be committed.
#36
@izmeez: argh, my fault. i attach the same patch.
@matt2000: put up new patch without div.
kindly to re-test.
#37
just for additional references, i put some screenshot for reason i put DIV in the last send text.
ps: this doesn't mean i want it to be commit just to tell in clear story manner.
#38
@parasolx Thanks for putting up both patches with and without div. Also thanks for sending me an email showing me how adding the div moves the button to the next line. It does look better.
I am happy to accept whichever the module maintainer wishes.
#39
@parasolx The same can be achieved without a DIV tag by changing the one line in the patch to the following:
'#value' => t('Last Send: !date <br />', array('!date' => $notify->send_last > 0 ? date('n/j/y g:i a', $notify->send_last) : "never")),#40
Just another two cents from me, personally I find the code to be more readable when the $form['notify_send_last'] comes after $form['uid'] the way it was in the original patch in comment #6. This way the code for the text is followed by that for the submit button.
$form['uid'] = array('#type' => 'value', '#value' => $account->uid);+ $form['notify_send_last'] = array('#type' => 'markup',
+ '#title' => t('Notify last send'),
+ '#description' => t('The date of the last notification for this user.'),
+ '#value' => t('Last Send: !date <br />', array('!date' => $notify->send_last > 0 ? date('n/j/y g:i a', $notify->send_last) : "never")),
+ );
$form['submit'] = array('#type' => 'submit', '#value' => t('Save settings'));
just a minor point that others may see differently.
#41
@izmeez: thanks for that. actually the main reason I change the last send text above
$form['uid'] = array('#type' => 'value', '#value' => $account->uid);because it still look same as without div and to make arrange code more easier to look.
since this module does not provide any CSS files, adding div will definitely make the last send text to be break into new line from submit button. of course appearance will difference based on what theme was used.
but the idea to add DIV because in modern browser it has attribute of display:block as default without any insertion of inline style (which I want to avoid it cause inline style can reduce certain performance). The idea is to add it so we can have new line break for submit button what ever theme site is using.