Don't create session var when not masqerading

What pages are slower? Can you install the devel module to determine if it's a SQL or PHP issue? And is the issue present in the latest -dev version?

Hi,
I'm also having this problem on a high traffic site

Part of the issue is that masquerade sets a session cookie for all users - which effectively disables varnish caching.

It also makes a database query for every page view for every user

function masquerade_init() {
  global $user;

  // load from table uid + session id
  $uid = db_result(db_query("SELECT uid_from FROM {masquerade} WHERE sid = '%s' AND uid_as = %d", session_id(), $user->uid));
  // using if so that we get unset rather than false if not masqing
  if ($uid) {
    $_SESSION['masquerading'] = $uid;
  }
  else {
    $_SESSION['masquerading'] = null;
  }
}

I wonder if it would work to set a cookie to indicate masquerading ?

The cookie should still be available in the new masqueraded session - and would enable the hook_init to be dropped.

So when the user switches

* create a random ID
* save this to a db table along with the orginal user ID
* set cookie masquerade = random ID

Then check for this cookie in the masquerade block - looking up the user ID when appropriate.

Sorry I don't have a patch for this (for the time being we've just disabled masquerade) but I thought it might be worth noting the possible optimisation and seeing if people here think it's viable

Sean Burlington
www.practicalweb.co.uk

this killed us on a high traffic site, since it bypassed the caching we had set up to handle traffic. A non session based solution has to be found, otherwise I can't continue to use the module.

Since the session variable only needs to be availble for logged in users, can't the init hook be wrapped in a if (user_is_anonymous()) {} ?

I turned omerida's idea into a patch. Please try it. I don't have a high traffic site to see if it makes a difference. If someone +1, I'll commit it.

I'm not sure I understand why Varnish would allow using a cookie as per #4 while not a session variable (which requires a cookie as well). But if the suggestion in #5 gets around it, then I'm all for it.

Two thoughts about the patch in #6:

1. Currently, it's possible to use Masquerade to allow anonymous users to switch users. When I do it on 6.x-1.3, I get a notice, but I suppose it's possible for a site to be doing this. Even if we agree that anonymous masquerading shouldn't be allowed, the Drupal 6 permissions UI will still allow the permission to be assigned. Perhaps we could add a form_alter() on the permissions form to not allow that permission to be set? Still wouldn't fix API calls using permissions_api, but it'd be close enough. We'll need a pretty obvious update note to cover this change.
2. Let's clean up the comments while we're at it. // load from table uid + session id either needs to be removed or cleaned up, as it's pretty obvious that's happening in the line after. For // using if so that we get unset rather than false if not masqing I'm going to suggest:
   

   // Ensure that $_SESSION['masquerading'] is unset and not FALSE if a user isn't masquerading.

Subscribing. I've noticed that cache_page is never written to whilst using Pressflow if Masquerade is enabled.

@muhleder, it would be great if you could try the patch from #6.

I'm not trying to compete with deekayen's patch, but this is what we are using on our production site.

We, too, are using Pressflow with Varnish and had issues when masquerade is enabled, but where we noticed the most problems was with the session tables. Pressflow can be set to not save anonymous user's sessions, provided that there were no variables stored in the $_SESSION array. This is the patch that we have been using in order to keep anonymous sessions out of the database.

The main difference is that it includes a condition to disallow masquerading as an anonymous user, which is necessary if we are no longer checking for masquerading when the user is anonymous.

There was at one time a maintainer that removed the functionality to be able to masquerade as anonymous. We could return to that. The alternative is logging out and back in, of course.

I'm OK with committing a patch that makes Pressflow work and breaks anonymous masquerading, but #6 and 10 don't quite document those realities and seem more of a hacky workaround.

minor update to #10 above to make it apply cleanly on new 6.x-1.4 version of masquerade

Suppose anonymous masquerading (ability to masquerade from anonymous to registered user) should be fixed with form_alter() of permissions page.

patch #6 is more reasonable and self commented

Ability to masquerade as anonymous user seems useless because pages for anonymous mostly cached and I see no ability to display a "switch back" link.

Another idea is make setting: Allow masquerade as anonymous. Than hook_init() should check this variable, check that session started, and in case that user is masquerading as anonymous disables page caching by $conf['cache'] = FALSE;

I'm using #12 at the moment, works great - no more session cookie. Thanks!

Patch #12 worked great for me. No more Googlebot inflating the guest users count because I have all the anon sessions tagged with "masquerading|N;"

+1 for the patch in #6. The coding style seems nicer to me and both do the same. So I remade it against latest head. This is currently runnig without problems at two of our sites. One gets 5.000 Visitors a day.

I just wanted to chime in to say how important it is that this module gets fixed or at least put up a big warning that says it will destroy your site's performance.

Two years ago I started a social news site running Drupal where people post stories and vote on them. I've worked full-time on this site since then, and though we've seen thousands of people sign up and have a loyal community, we were never able to get page load times on the site under 8-15 seconds per page. It didn't make sense.

A year into it, I decided we needed to get the problem fixed. So I hired a developer to setup Nginx and PressFlow, to put our DB on one server and our web stuff on another. I've upgraded our servers big time. I've also hired in about 4 other developers, each who promised to solve our speed problem only conclude that it was a random piece of bad code somewhere in our setup that was killing things and that they were not interested in hunting for it.

These changes brought our page loads down to about 7-8 seconds. Still unacceptably slow. It's been heartbreaking and has probably cost me about $15,000 cash plus who knows how many lost readers/users.

I get invited to a fair number of conferences where people in the media ask me about using Drupal for their new projects. I've spent the last year telling people to run far far away from Drupal as it's terribly slow no matter what sensible thing you do to it. I still use it because the switching costs would be too high at this point, but I've told everyone I know never to touch the thing if they want to do anything serious with it.

And here tonight I just stumbled onto this thread and found the answer to why our sessions table is so out of control. Found the answer to why the money I spent to hire a guy to set up Pressflow was all wasted. The Masquerade module was killing it all. This was the random broken code that has made life so difficult these past few years.

I've installed the latest dev release and our site it fast as lightning. It's a bit of a revelation.

Thank you to those who started this thread and posted fixes. To the maintainer, I can't urge you strongly enough to commit these changes into production releases so that no one else will suffer from running the previous version of the module.

I'll keep an eye on performance to see how things go, but so far, this has been a real eye-opener and an answer.

Committed #16 to DRUPAL-6--1. HEAD still needs someone to look at it.

This is a duplicate of #908194: Needless session creation = incompatible with Pressflow. Closing...

uhhh....

This one is older, so actually the other one is a duplicate of this, & standard Drupal procedure would be to link from that one to this one.

There is a proposed revision to the implementation of this at #908194-5: Needless session creation = incompatible with Pressflow that would maintain masquerading as Anonymous, but I think unsetting an unset variable is a safe thing to do. I also want to stick with either using $GLOBALS or declaring global $user. I don't want every patch to be flip flopping that.

Awesome stuff, guys.

OK but changed the title to be more descriptive.

... but I think unsetting an unset variable is a safe thing to do.

I cannot test what's going on in a PHP 5.3 environment. It was maybe a non necessary precaution. Maybe someone can test the patch on PHP 5.3.

I also want to stick with either using $GLOBALS or declaring global $user

global $user; is a common practice in Drupal world. So, remaining on that.

I committed #23.

subscribing

Sorry

This code already in D7 branch. Suppose we should cleanup code for session management.

We are using $_SESSION[masquerading] and $user->masquerading suppose $_SESSION is a preferable place and we just need _masquerade_is_masquerading() function .

+++ masquerade.module	30 Oct 2010 08:47:32 -0000
@@ -32,17 +32,19 @@ function masquerade_perm() {
+    unset($_SESSION['masquerading']);

This could initialize a new session so been fixed in #987610: Notice: Undefined variable: _SESSION in masquerade_init

Powered by Dreditor.

Subscribing.

Subscribe

Variables are not initialized with isset() so let's continue in #1103588: Cleanup code and optimize query count

I looked at commits back to February and none of them reference this. Was it fixed in 7.x only or 6.x as well?

Thanks for your work on the module.

I think #24 point to commit, 6.x already fixed