Add ability to disable locking for bots only

[convenient]

In New Relic I frequently see slow requests stuck trying to read the session due to being caught on the lock. Most of the time these are from bots as they crawl the site at speed.



I was thinking that the artificially slow request could potentially have some negative SEO effects, especially on Product pages and similar.



If we had the ability to disable session locking for bots only we could prevent this issue.

I have not had success previously with disabling session locking entirely so was thinking a little more granular control at this level could help out.

Let me know what you think of this idea.

[colinmollenhour]

I'm a little hesitant to add this because it would be a breaking change (new interface method) and that the same could already be accomplished by adding some code to your bootstrap process like:

if (preg_match('/..../', $userAgent)) {
    define('CM_REDISSESSION_LOCKING_ENABLED', FALSE)
}

[convenient]

That sounds very much like what I'm looking for. can you direct me to where CM_REDISSESSION_LOCKING_ENABLED is picked up in code please?

[colinmollenhour]

Uh oh, looks like this functionality got lost when the library was abstracted for the Magento 2 module and now it is only mentioned in the Magento 1 module readme...

Are you using one of the Magento modules or are you using this in a non-Magento project? Since getDisableLocking is a method already it could contain any logic you wanted for disabling locking.

[convenient]

@colinmollenhour Uh oh.

I'm using this in Magento 2.

Are you suggesting that https://github.com/magento/magento2/blob/2.3.0/lib/internal/Magento/Framework/Session/SaveHandler/Redis/Config.php#L276 could be updated to be more like the following?

public function getDisableLocking()
{
    if ($this->deploymentConfig->get(self::PARAM_DISABLE_LOCKING)) {
        return true;
    }

    if ($this->deploymentConfig->get(self::PARAM_BOT_DISABLE_LOCKING)) {
        $userAgent = empty($_SERVER['HTTP_USER_AGENT']) ? '' : $_SERVER['HTTP_USER_AGENT'];
        if (preg_match('/..../', $userAgent)) {
            return true;
        }
    }

    return false;
}

This would not be backwards breaking to this modules interface, but would add additional magento only configuration?

[colinmollenhour]

Yes, your example is along the lines of what I was suggesting. The original proposal was also fine but would require a major version bump to avoid breaking existing implementations.

[convenient]

@colinmollenhour No worries I completely get the need to prevent breaking changes. I just wanted to demonstrate what I was trying to achieve and open a dialogue.

So similar to not wanting to break this module modifying the magento core is also something I'd want to avoid. However I think I can jury-rig this functionality as so

In /path/to/magento2/composer.json

  "autoload": {
    "files": [
      "app/etc/cm-redis-session-disable-locking.php"
    ],

In app/etc/cm-redis-session-disable-locking.php

<?php
$userAgent = empty($_SERVER['HTTP_USER_AGENT']) ? '' : $_SERVER['HTTP_USER_AGENT'];
if (preg_match('/^alexa|^blitz\.io|bot|^browsermob|crawl|^curl|^facebookexternalhit|feed|google web preview|^ia_archiver|indexer|^java|jakarta|^libwww-perl|^load impact|^magespeedtest|monitor|^Mozilla$|nagios |^\.net|^pinterest|postrank|slurp|spider|uptime|^wget|yandex|^elb-healthchecker|binglocalsearch/i', $userAgent)) {
    define("CM_REDISSESSION_DISABLE_LOCKING", "1");
} else {
    define("CM_REDISSESSION_DISABLE_LOCKING", "0");
}

In app/etc/env.php

  array (
    'save' => 'redis',
    'redis' => 
    array (
        'disable_locking' => CM_REDISSESSION_DISABLE_LOCKING,

The above should allow me to define the configuration on the fly, without hacking the Magento2 core or breaking this modules compatibility. Do you think that would work 🤔

[colinmollenhour]

I'm not as familiar with Magento 2. I'd just double-check that the result of app/etc/env.php is not cached and reused for the next request, otherwise looks like a good approach!