In uc_roles_cron(), the entire role expiration table is loaded and looped through via a foreach(). The first thing the loop does is load the user account associated with the expiration. Thus, any website with significant volume will be loading thousands of users every cron run just to see if an expiration notice needs to be sent out. Can this query not be refined to only load expirations that require action?

Behold the glory of uc_roles.module lines 42-44:

  $result = db_query("SELECT * FROM {uc_roles_expirations}");
  while ($expiration = db_fetch_object($result)) {
    $account = user_load($expiration->uid);
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Rhino’s picture

I have this issue. uc_roles never runs via cron successfully. Since upgrades and role assignments were working fine, I never noticed that this was happening until cron started acting badly.

torgosPizza’s picture

Good god. Yes, this query should filter by the `expiration` field at the very least, since it looks like that's a unix timestamp int(11) in the table.

inforeto’s picture

subscribing

torgosPizza’s picture

Going to be looking into this soon, so I'm giving a friendly bump.

Offlein’s picture

Uh. +1?

Liliplanet’s picture

+1 for D7 .. thx :)

TR’s picture

#1402106: uc_roles_cron line 42 property of a non-object also needs to be addressed by any patch that rewrites uc_roles_cron().

Offlein’s picture

Status: Active » Needs review
FileSize
2.27 KB

Hi, here's a patch I wrote. It incorporates the stuff from issue #1402106.

I do a few things here. For one, I establish a single $curtime variable as a "Start time" because I was afraid of (I guess) race conditions being introduced by calling time() at different stages throughout the function and missing something by like a single second or two.

Then, I simply add a "WHERE expiration <= $curtime" clause, to limit the query only to users whose roles are expiring.

Finally, I check to see if reminders are enabled (if $reminded_granularity != 'never') and, if they are, I get the timestamp for the absolute maximum UNIX timestamp in the future that a row could be relevant. (That is to say, if reminders are set to 3 days, I will find the timestamp for exactly 3 days from $curtime.) Then I add an OR statement onto my new WHERE clause and say, basically, also SELECT rows where the expiration column is <= that timestamp, as long as the notified column IS NULL. (We don't want to select rows that were already notified. I remove the check that existed for this later in the code, as it's now redundant.)

Then, as I said, I just include the code to check if the user account has been disabled.

Hope this makes sense! Please test it as soon as possible. On my site, as expected, my next query went from polling 540-something users to .. 1. What's the performance increase there?

rickmanelius’s picture

Can't believe I missed this one and posted here instead #1413584: Split up uc_roles_cron.

I'll test #8 as soon as I get a moment as this will greatly reduce the load on my servers!

TR’s picture

Sounds like a reasonable approach. I took a look at the patch and it looks good, but I haven't had a chance to test it yet - I don't have a test site with a large number of roles expirations so it will take me a little work to make a proper test.

rickmanelius’s picture

There are a few differences for D7... so I'm submitting a slightly modified version of #8 for D7.

Status: Needs review » Needs work

The last submitted patch, rewrite_uc_roles_cron-911350-11.patch, failed testing.

Offlein’s picture

Status: Needs work » Needs review

This should not affect the 6.x patch, yeh? Needs review still.

rickmanelius’s picture

Yup, they both need review.

I know my patch failed the test bot, but I think it's because I didn't remove the leading directory names in the file paths. I can reroll or you can simply patch and test it as well.

longwave’s picture

Ideally I would like to see some tests for this. I already added a basic test case for role purchases to the 7.x branch, which could be expanded to cover expiration tests.

rickmanelius’s picture

Gotcha. In terms of real world test, I applied my D7 patch and ran it yesterday, successfully processing 40 recurring payments in a single run and it was significantly quicker.

But I agree that a test would be even better. Unfortunately, that's a Drupal milestone I haven't crossed yet! But this may inspire me to finally jump in there...

longwave’s picture

I don't think we can test the performance here, but it would be good to confirm that a role expires when expected, or can be renewed and then also expires after the correct extended time; if this works both before and after the patch, it helps to prove that the changes here don't break anything. I think the difficult part will be "time travelling" from within the test harness.

TR’s picture

@rickmanelius: Your patch failed the testbot because this issue is for 6.x-2.x and your patch is for 7.x-3.x. Your patch will not apply to 6.x-2.x. Please rename your patch according to the instructions next to "Attach a new file" below:

For patches that apply specifically to a version of core other than the one the issue is pointing to, you can prefix the extension with -D(number) to prevent them from being queued for testing.

rickmanelius’s picture

Thanks for the tip @TR!

Resubmitting #11

peterx’s picture

expiration could use an index to prevent a table scan.

Status: Needs review » Needs work

The last submitted patch, rewrite_uc_roles_cron-911350-19-D7.patch, failed testing.

peterx’s picture

D7 uc_roles_delete expects an account object. I use:

    if (!$account) {
    	$account = (object) array('uid' => $expiration->uid);
      uc_roles_delete($account, $expiration->rid);
      continue;
    }
rickmanelius’s picture

Status: Needs work » Needs review
FileSize
1.27 KB

Not ignoring #20 and #22. Merely fixing the patch in #19 to pass the testbot. Others are more than welcome to modify/adapt from here!

Status: Needs review » Needs work

The last submitted patch, rewrite_uc_roles_cron-911350-23-D7.patch, failed testing.

pepperoni13’s picture

+1

TR’s picture

Sigh. Please don't post just to subscribe to an issue. If you want to follow the issue, just press that big green "Follow" button in the upper right of the screen. That's what it's there for.

And if you're really like to see this resolved, you could maybe help out by testing proposed patches or even contributing code.

rickmanelius’s picture

Just FYI, I've been using this on production for almost 1.5 months now with no issues.

pepperoni13’s picture

Sorry TR ... Newb move as I didn't realize they had added that feature ... I like it!

Anyways, added #23 (v7.x), Thanks rickmanelius seems to do the trick. Notify mesage from the reports "Notice: Trying to get property of non-object in uc_roles_cron() (line 42 of uc_roles.module)." went away.

Thats the good, now the bad, got the messages: (4x each)
Notice: Trying to get property of non-object in _uc_roles_flush_menu_cache() (line 1343 uc_roles.module.
Notice: Trying to get property of non-object in uc_roles_delete() (line 1127 uc_roles.module.

I Found out that I had 4 entries in the uc_roles_expirations table with deleted users attached to them, deleted them manually ... no more notifications. Probably why I recieved these notifications previously, now wondering if the notifications came up due to them not being removed within the cron. Should running the cron remove those entries?

btw rickmanelius, can you post the code for uc_roles_cron function or add the uc_roles.module your using in case they differ any?

Willing to help if need be. I can change a little code etc, but not a php programmer and test etc.

rickmanelius’s picture

Hi pepperoni13.

I can confirm that some of the errors you see in #28 are showing up on mine as well. I can test and resubmit a new patch. Thanks for the feedback!

anrikun’s picture

Status: Needs work » Needs review
FileSize
2.05 KB

Patch at #8 works but does not apply to 6.x-2.x-dev any more.
Also it can be simplified.
Here is an updated version.

anrikun’s picture

Component: Other » Roles
FileSize
2.05 KB

There was a SQL typo in previous patch. Sorry for that!

rickmanelius’s picture

Version: 6.x-2.x-dev » 7.x-3.2
FileSize
957 bytes

Re-rolling the patch for D7 from patch #23. Hunk 2 in patch #23 is no longer necessary as the lines just below it account for a role that is already deleted.

Status: Needs review » Needs work

The last submitted patch, rewrite_uc_roles_cron-911350-32.patch, failed testing.

anrikun’s picture

Version: 7.x-3.2 » 7.x-3.x-dev
Status: Needs work » Needs review
FileSize
1.94 KB

I give it a try too.
It's the D7 port of patch at #31.

rickmanelius’s picture

Hmm... well if we're going to make it fully D7, perhaps the first chunk in #34 also gets replaced with the DBTNG format??

longwave’s picture

I have not yet tested this, but reading #34 the second part of the patch no longer checks $reminder_granularity != 'never' or $expiration->notified, so it looks like it might repeatedly send reminders to everyone if reminders are disabled? Why is the second part of the patch needed?

rickmanelius’s picture

Hi @longwave. Good point. My suggestions:

1. The point of this ticket is to reduce the scalability of the cron hook by reducing the number accounts being looped against to only those that are within an applicable time window. Therefore it might be better to split anything else off into a different ticket and focus solely on the scalability.

2. Unfortunately I didn't thoroughly check the path in #34 before commenting because I do see your point. I think the check for $expiration->notified is still necessary but the check reminder granularity might not be necessary because the query would have already have filtered that out. Still, it doesn't hurt to keep it to keep the logic understandable.

anrikun’s picture

@longwave, rickmanelius, here is an updated patch that takes into account your remarks:
- It checks $reminder_granularity != 'never'. I had removed it in my previous patch and you're right, it's still needed.
- Query uses the DBTNG format.

anrikun’s picture

Oops sorry, posted wrong patch. Here's the correct one.

anrikun’s picture

I've updated the patch for D6 accordingly.

longwave’s picture

Status: Needs review » Fixed

Committed #39 and #40. Thanks to everyone who contributed to fixing this issue!

Status: Fixed » Closed (fixed)
Issue tags: -scalability

Automatically closed -- issue fixed for 2 weeks with no activity.