wpsharks / comment-mail

A WordPress plugin enabling email subscriptions for comments.
http://comment-mail.com
GNU General Public License v3.0
8 stars 3 forks source link

Queued (Pending) Notifications are never sent #192

Closed raamdev closed 7 years ago

raamdev commented 8 years ago

I was testing Comment Mail Lite v151224 after importing StCR subscriptions when I tried posting two new replies to existing comments with subscriptions (i.e., I posted new replies to comments that had StCR subscriptions, which were imported into Comment Mail). The new Comment Mail subscriptions were created fine, and were connected to the appropriate imported StCR subscription, however the notifications for those new replies were never sent.

Looking at the Mail Queue, I noticed the two notifications were sitting there, unsent:

2015-12-26_09-37-26

Notice the time on them... it had been ~45 minutes since the notifications were created and they still have not been sent. Here's what I tried to get the email queue to be processed:

None of these things helped. The queued notifications remained queued and were not sent out.

Also, my PHP error log did not report any errors during this time.

bobinoz commented 8 years ago

Hi everybody

I just wondered if you are still working on this? I've got to the stage where I need to find a solution, because at the moment the front end user experience on my website is not great for my readers with lots of pages timing out after they make comments. I am now considering going through my subscribers on StCR and just randomly deleting some users (not ideal) from the system just to speed up things. Before I take that drastic action though, just wondered if there were any updates?

Thanks, Bob

raamdev commented 8 years ago

@bobinoz Yes, we're still working on this. It's a high-priority issue for the next release. I'll be digging into this issue this week.

bobinoz commented 8 years ago

Sounds good raamdev, and thanks for the quick response. I will hold fire on that subscriber cull then and wait to hear from you further. Thanks, Bob

jaswrks commented 8 years ago

That seems like a clue to me. If they go through at all, then it must not be a parsing issue (otherwise they would never go through) but instead something related to time...

If you believe it to be an issue with time, then it would almost certainly show up at this line; i.e., this would be a good place to begin another round of debugging. https://github.com/websharks/comment-mail-pro/blob/160618/src/includes/classes/QueueProcessor.php#L244

So for instance, if there are messages in the queue and you look in the DB and find that all criteria suggests that they should be sent on the next queue process, but the next queue process runs, and they are not sent, then it was almost certainly because of this line.

If not there, then here, where the query either will or will not include them when processing begins.


I also wouldn't give up on the idea that it could be related to a parsing issue in the template file that is triggered by some special circumstance that I haven't seen yet.

It would be easier to debug this if templates were not being eval'd. In PHP 7, errors in eval() can be caught as an exception, so it's much easier. In older versions of PHP, eval'd code that triggers an error gives you very little to go on.

I feel it would be an improvement in Comment Mail if we stopped using eval() for template files, and instead include() them in an isolated way. So for instance, if you look at this line, you can see it currently calls upon ->evaluate() and it passes a set of $vars.

That could be changed so that instead of calling ->evaluate(), we call a method that would include the file, buffer the output, and return the result. No different than what we do now, but minus a call to eval(), which will improve compatibility, security, and make it easier to catch errors when they occur.

function isolatedInclude($file_path, array $vars) {
    extract($vars);
    ob_start();
    include $file_path;
    return ob_get_clean();
}
jaswrks commented 8 years ago

↑ That example could be cleaned up a bit. For instance, it would be best to avoid polluting the currently defined variables with the function parameters themselves; i.e., $file_path and $vars. Those could renamed to something that would be less likely to conflict with what is extracted from $vars; e.g., $___file_path, $___vars.

jaswrks commented 8 years ago

My feeling at the moment is that this is related to a template parsing issue that we haven't caught yet. Why? Because I went over the QueueProcessor class carefully about a month ago, did a video about it, and put some time into trying to debug it myself. If there's a bug related to time it would surprise me; i.e., I'm not seeing it.

That's not to say there's not a bug. There very well could be. I'd just be surprised to see that be the case. For that matter, I haven't been able to reproduce this at all yet myself. However, based on the helpful reporting you've been doing here, it would seem more likely to me that it's a template parsing issue that results in an error, which shuts down the script in an unexpected way.

bobinoz commented 8 years ago

Hi everyone

Sorry to trouble you all again, but I noticed there was an update to this plug-in a short while ago. I assume, because there's been no update here, that this issue has not yet resolved?

So I wondered if you had any updates on this? Thanks again, Bob

raamdev commented 8 years ago

@bobinoz That's correct. The latest Comment Mail release did not include any changes from this issue, but this is still a high-priority issue and the next release will hopefully have this issue resolved.

bobinoz commented 8 years ago

Okay, thanks for the update, I do appreciate it.

aptharsia commented 8 years ago

Add me to the list of having this bug. Hope this is resolved soon. BTW, my database is over 26,000 subscriptions (not as in that's how many emails are being sent at once, just total.)

raamdev commented 8 years ago

@aptharsia Thanks for the feedback! This issue is still very much in progress and I'm hoping to get it closed for the next release.

IvanRF commented 7 years ago

@raamdev did you find a way to manually execute some PHP in order to process the Queued (Pending) Notifications? I just tried WP Crontrol to run the cron job, but it didn't work.

raamdev commented 7 years ago

@IvanRF No, that's the main problem of this issue: Some Queued (Pending) Notifications are not getting processed as they should when the appropriate cron runs.

IvanRF commented 7 years ago

I solved my issue 😃

I started digging, tried chaning hold_until_time on the database, check the code and finally found with WP Crontrol that the cron job _cron_comment_mail_queue_processor was not present. I missed that yesterday.

So, to solve it, I just runned on WP Crontrol (Add PHP Cron Event option) this code I found in your code:

wp_clear_scheduled_hook('_cron_comment_mail_queue_processor');
wp_schedule_event(time() + 60, 'every5m', '_cron_comment_mail_queue_processor');

Now, the cron job is there and it is processing mails again.

raamdev commented 7 years ago

@IvanRF That's odd. There is no scenario in which the _cron_comment_mail_queue_processor cron event should ever get deleted (other than when you uninstall Comment Mail).

IvanRF commented 7 years ago

It will be useful to know if the other users having this issue have this cron job running or not.

bobinoz commented 7 years ago

It will be useful to know if the other users having this issue have this cron job running or not.

I would be happy to supply this information if someone could point me in the right direction to find out, although I do currently have Comment Mail deactivated.

IvanRF commented 7 years ago

@bobinoz you can use the WP Crontrol plugin and go to Cron Events; or if you have Wordfence go to Diagnostics > Cron Jobs.

bobinoz commented 7 years ago

Okay, I've installed WP Crontrol but of course the Cron job isn't showing, I have Comment Mail deactivated. If I activate it, then it will automatically deactivate StCR which I am currently using, so I'm a little concerned about the implications of that.

Advice needed; if I just quickly activate Comment Mail and then look in the Cron jobs should I be able to see it immediately? Once I know the answer to the question, if I then deactivate Comment Mail again will StCR automatically kick in again with no harm done?

raamdev commented 7 years ago

@bobinoz writes...

Advice needed; if I just quickly activate Comment Mail and then look in the Cron jobs should I be able to see it immediately?

Yes, it should appear immediately after Comment Mail is activated.

Once I know the answer to the question, if I then deactivate Comment Mail again will StCR automatically kick in again with no harm done?

Yes, correct. Comment Mail won't touch the StCR data in any way. You can simply deactivate Comment Mail and then make sure StCR is enabled and you'll be back to the way you were.

bobinoz commented 7 years ago

Okay, I had a look at this today, and with Comment Mail deactivated I had the following to Cron jobs showing in the WP Crontrol plug-in:

_cron_comment_mail_sub_cleaner None

_cron_comment_mail_log_cleaner None

I then disabled StCR and enabled Comment Mail and looked inside the Cron job events again and found:

_cron_comment_mail_queue_processor None

_cron_comment_mail_sub_cleaner None

_cron_comment_mail_log_cleaner None

I have now deactivated Comment Mail again and activated StCR and checked again in the Cron job list and the same three Cron jobs as above are still showing.

Hope that helps, thanks, Bob

IvanRF commented 7 years ago

@bobinoz what I can tell from that is that this plugin does not remove the cron jobs on deactivating, but more importantly that your original issue was the same as mine: _cron_comment_mail_queue_processor was not running. Now, @raamdev told me this was fixed on v160618 (see #194). So, I guess that if this happened to you while running a previous version, you will have no issues with the latest version.

raamdev commented 7 years ago

@bobinoz Can you tell me which version of Comment Mail you ran that test with?

bobinoz commented 7 years ago

@raamdev Version 160824

jaswrks commented 7 years ago

Flagging the objective in this issue, at least for part 1, as what was outlined here: https://github.com/websharks/comment-mail/issues/192#issuecomment-234367442

Estimate at 1 day.

raamdev commented 7 years ago

@bobinoz Thank you.

@IvanRF My feeling is that the issue you reported with the cron job is unrelated to the core issue described and researched in this GitHub issue, which is related to the mystery that in some cases notifications get stuck in the Queued (Pending) state. If you feel there is an issue related to the cron job, please open a separate GitHub issue with a list of steps to reproduce the problem so that we can add that to the list of things that need research.

@jaswsinc writes...

Flagging the objective in this issue, at least for part 1, as what was outlined here: https://github.com/websharks/comment-mail/issues/192#issuecomment-234367442

Estimate at 1 day.

Awesome. Thanks! I'm keeping this on the Next Release milestone so that we can get this long-standing issue fixed, hopefully once and for all! :-)

jaswrks commented 7 years ago

Next Release Changelog:

raamdev commented 7 years ago

@jaswsinc You marked this as partial completion; could you clarify? Is it 'partial' because it still needs to be tested to confirm the original issue has been resolved or are you suspecting it has not been resolved and more work will be needed once we get a chance to debug again?

jaswrks commented 7 years ago

@raamdev Partial because this change only makes it easier for us to debug (i.e., discover) what is causing this problem on some installations. In short, the change documented for the next release does nothing to correct the problem, it only makes it easier for us to discover what may be causing it. As I understand it, none of us have been able to detect what actually causes this yet. My hope is that better template parsing will get us one step closer.

raamdev commented 7 years ago

@jaswsinc I just ran the latest from the dev branch (which includes your latest changes to the way templates are parsed) and following the steps I outlined above, I have been unable to reproduce the original issue outlined here, i.e., messages are no longer being stuck in the queue!

@lucashall @bobinoz @aptharsia @IvanRF If any of you would be open to testing this latest fix to see if it fixes the stuck "awaiting processing" notifications in the Mail Queue (assuming you were having this issue), that would be awesome. Send an email to raam at websharks-inc dot com and I'll send you a copy of the latest dev release. Please mention this thread in your email.

bobinoz commented 7 years ago

@raamdev Yes, I'm happy to test it, and I've sent you an email.

aptharsia commented 7 years ago

Tried the dev release, still have the issue of notifications being stuck in processing mode and nothing gets sent.

raamdev commented 7 years ago

@aptharsia writes...

Tried the dev release, still have the issue of notifications being stuck in processing mode and nothing gets sent.

Are these old notifications that were in the queue before installing the dev version of Comment Mail, or did you install the dev version and then generate some new notifications to see if they'd get stuck? (In my test, I generated new notifications and they didn't get stuck like they had in previous tests.)

@bobinoz writes...

Yes, I'm happy to test it, and I've sent you an email.

I replied to your email.

aptharsia commented 7 years ago

I cleared any notifications still waiting, enabled and generated new notifications that were never sent out.

raamdev commented 7 years ago

@aptharsia Thank you for testing and reporting your results. I'll need to run some more tests to see if I can reproduce this again.

@bobinoz When you get the dev version of Comment Mail that I emailed you, it would be great to hear if you're still seeing this issue.

bobinoz commented 7 years ago

So I have installed Comment Mail and tested it briefly this afternoon, comments are still being held in the queued (pending) notifications area with the message 'n/a; awaiting processing'. However, they are getting sent, there just seems to be a long delay.

After I had installed and activated Comment Mail, I started answering comments on various pages with different numbers of subscribers. It sent out 76 emails immediately but then after that it started holding them in the queue for processing. What it seems to be doing since then is sending out little batches of emails every now and then at around 10 or 20 minute intervals. The most emails it seems to send out from the queue any one time is 21. Sometimes it is as few as 5. The maximum gap between sending them appears to be 30 minutes.

In the space of about seven hours, it has managed to send out all 363 emails in the queue which is now empty as it's late at night here and new comments have stopped coming in. If it were a busy day though, I can imagine the queue just building up until eventually it could take seven or eight hours before catching up with itself.

When I stopped answering comments, I think there were about 150 comments stuck in the queue, it has taken about three hours for those to slowly clear. So I'd say it's working now, but for some reason something is stopping it sending out all the comments at once. I am using SparkPost to deliver email, they do not have any hourly limits. As far as I'm aware, they just have a daily limit of 10,000 emails.

So if you have any clues how to fix it so they will get sent out immediately, we might be there.

I have noticed an unrelated problem as well, I hope you don't mind me mentioning it here. When I am logged in as admin but working on the front end, and I click on 'manage my subscriptions', with StCR I would see a list of subscribers for that particular post with tick boxes for them all so I could add, remove or delete them. When I click on 'manage my subscriptions' with Comment Mail, I don't see them at all, I just see that I am subscribed to all posts with my email address.

I can't find anywhere where I can view or amend subscriptions to a particular post or page. All I appear to be able to do is go to comment mail - subscriptions in the backend and view a complete list of subscribers. Is there anywhere I can go to quickly see all subscribers to a particular post or page?

Anyway, hope this information helps, and for now I am keeping Comment Mail activated. Thanks, Bob

raamdev commented 7 years ago

@bobinoz writes...

So I'd say it's working now

Woohoo! That's great to hear!

@bobinoz writes...

for some reason something is stopping it sending out all the comments at once.

Have you reviewed the Queue Processor Adjustments panel to see if those settings are appropriate? Comment Mail → Config. Options → Queue Processor Adjustments. You probably need to adjust those settings so that more emails get sent out at once.

@bobinoz writes...

I can't find anywhere where I can view or amend subscriptions to a particular post or page. All I appear to be able to do is go to comment mail - subscriptions in the backend and view a complete list of subscribers. Is there anywhere I can go to quickly see all subscribers to a particular post or page?

You can see a list of Comment Mail subscriptions for a post by editing the post itself and viewing the Comment Mail Meta Box, which will list all of the Comment Mail subscriptions associated with that post:

2016-11-10_13-28-55

Clicking "View All" at the bottom will bring you to the Comment Mail Subscriptions page, with the subscriptions filtered to just the subscriptions for that post.

aptharsia commented 7 years ago

Over about a 15 hour time span, 73 pending notifications only 12 were sent out. Many have been pending for over 12 hours. Nothing looks to be instant (or according to my settings) once moderator approved, for the 12 that got sent out, took 12 or so hours for the ones I timed. I wouldn't say it's fixed it so hope this issue will still be looked at.

aptharsia commented 7 years ago

Adding. The 12 that were sent has a 14 hour difference between the two sets sent yet there are 15 hour waits queued to be sent so it looks random on what gets sent and what queues for processing.

Edited: I noticed it was 6 notifications sent out the two times it happened with 14 hours between the two sets.

raamdev commented 7 years ago

@aptharsia Thank you for the additional information. I'll be keeping this issue open until I can run some more tests and get to the bottom of this. The template parsing changes that Jason made related to this issue should help us with debugging. I just need to work at reproducing this issue in a test install because my test install that previously wasn't working is now suddenly working, so I need to keep testing.

jaswrks commented 7 years ago

@aptharsia After the recent changes in this issue, what we are now looking for to diagnose the why, are your PHP error logs. If you can check with your hosting company or already have access to these logs, that would be much appreciated.

To clarify, with the improved template parsing as of late, we'd like to inspect any PHP errors being reported when this occurs, as that could shed light on a real solution; i.e., help us understand under what conditions this occurs and what error causes the queue to get stuck.

I have followed the steps that you outlined on a few different occasions now, and have been unable to reproduce. So whatever causes this remains a mystery for now. Until we can see PHP error logs that might unveil the underlying cause, I'm afraid this could remain open.

If you check your PHP error logs and find nothing, we'd like to know that also.

bobinoz commented 7 years ago

@raamdev Thanks for the information, yes, I can now see where I can view and amend subscriptions associated with particular posts and pages.

I've also check the settings you mention Comment Mail → Config. Options → Queue Processor Adjustments, but cannot make changes as that's only available in the pro version. However, the settings seem good to me, if I am reading it correctly it should send 100 emails at a time and do that every five minutes.

cm4

Any idea why it isn't able to do that?

raamdev commented 7 years ago

@bobinoz Please open a separate GitHub issue for that, so that we don't go off topic in this one.

aptharsia commented 7 years ago

Is the _cron_comment_mail_queue_processor in WP Cron suppose to have a hook or argument or is that suppose to be blank? I've been watching the PHP error logs but nothing has shown since the last run 5 hours ago.

raamdev commented 7 years ago

@aptharsia There are no arguments, no. See screenshot below from WP Crontrol:

2016-11-14_19-13-17

aptharsia commented 7 years ago

23 hours since last notifications sent, the rest are still in the queue. Nothing is on the error log so far, does that mean it's not even trying to send them all this time?

aptharsia commented 7 years ago

I spoke too soon. It finally did another batch after 24 hours. I checked the error log and no errors for this plugin. Yesterday it did it twice with 25 minutes between the two batches and then nothing until 24 hours later.

raamdev commented 7 years ago

@aptharsia Could it be that the notifications were notifications that were part of a Daily Digest subscription that were supposed to be held for 24 hours?

renzms commented 7 years ago

@raamdev

Was unable to reproduce original issue. Notifications eventually sent in batches.

aptharsia commented 7 years ago

@raamdev No, I checked and everyone is instant, no digests. There is inconsistency with when they are being sent out. Yesterday was 24 hours, today there was 16 between the most recent and last time sent.

Edited: After 16 hours, 8 emails were sent out. Then after only 8 minutes 5 emails were sent out (although there were many more queued.)