Cm_RedisSession Module Rewrites Session Handler

[rvelhote]

Preconditions (*)

1. Have the session handler configured as <session_save>db</session_save>
2. Update to OpenMage 20.1.0 using composer

Steps to reproduce (*)

1. Have the session handler configured as <session_save>db</session_save>
2. Update to OpenMage 20.1.0 using composer
3. Clear the core_session table (not required but makes it easier to check the result)
4. Browse your store, add things to cart

Expected result (*)

1. Sessions should be stored in the database and not in Redis. OpenMage should respect the session_save configuration

Actual result (*)

1. The core_session does not have any new sessions (because they are now in Redis)
2. The session handler gets rewritten by the Cm_RedisSession module and the database handler is ignored. The module Cm_RedisSession is installed and enabled by default.

[fballiano]

did you open the issue in @colinmollenhour repo at https://github.com/colinmollenhour/Cm_RedisSession?

[rvelhote]

I can open it there as well, no problem. I didn't because I thought it's primarily a discussion to have within the OpenMage context. It can also be helpful to people that face the same issue when upgrading.

[fballiano]

now that the Cm_RedisSession code is not in our repo, probably we wouldn't even have the possibility to fix it

[colinmollenhour]

Ahh, I think this relates to https://github.com/colinmollenhour/Cm_RedisSession/pull/190..

[fballiano]

@colinmollenhour I can confirm that I can reproduce the problem.

my local.xml has no redis configurations at all: <session_save><![CDATA[db]]></session_save>

but somehow the sessions get saved on redis (which I've running on my machine) anyway

[colinmollenhour]

@fballiano Do you have a <redis_session> element in your config?

[fballiano]

@colinmollenhour noup, this is my whole local.xml

<config>
    <global>
        <install>
            <date><![CDATA[Fri, 07 Jul 2023 19:18:55 +0000]]></date>
        </install>
        <crypt>
            <key><![CDATA[540b96d9f2e4b7e7245f063e1b9349d5]]></key>
        </crypt>
        <disable_local_modules>false</disable_local_modules>
        <resources>
            <db>
                <table_prefix><![CDATA[]]></table_prefix>
            </db>
            <default_setup>
                <connection>
                    <host><![CDATA[localhost]]></host>
                    <username><![CDATA[X]]></username>
                    <password><![CDATA[X]]></password>
                    <dbname><![CDATA[X]]></dbname>
                    <initStatements><![CDATA[SET NAMES utf8]]></initStatements>
                    <model><![CDATA[mysql4]]></model>
                    <type><![CDATA[pdo_mysql]]></type>
                    <pdoType><![CDATA[]]></pdoType>
                    <active>1</active>
                </connection>
            </default_setup>
        </resources>
        <session_save><![CDATA[db]]></session_save>
    </global>
    <admin>
        <routers>
            <adminhtml>
                <args>
                    <frontName><![CDATA[admin]]></frontName>
                </args>
            </adminhtml>
        </routers>
    </admin>
</config>

[colinmollenhour]

@rvelhote I think as a quick workaround you could override the rewrite in your local.xml:

  <global>
    <models>
      <core_resource>
        <rewrite>
          <session>Mage_Core_Model_Resource_Session</session>
        </rewrite>
      </core_resource>
    </models>
  </global>

[colinmollenhour]

I could remove the rewrite again and release it as version 3.2.0 with a note in the README to add the rewrite to local.xml where needed, but non-OpenMage users or non-updated users that don't have the version fixed in composer.json and people using the repo directly would be affected unexpectedly since the rewrite would be missing.

That said, does the db adapter really add any value? For small installations the files adapter is jsut as good or better and for large (multi-node or high-traffic) installations the Redis adapter is much better than the MySQL one. Is there a use-case I'm missing for the 'db' adapter?

[rvelhote]

@colinmollenhour @fballiano Thank you for the quick help. I was already discussing this topic in Discord and I ended up using a composer patch to disable the module rather than rewriting the rewrite (fits our workflow better).

Index: vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml b/vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml
--- a/vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml
+++ b/vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml    (date 1692697724130)
@@ -33,7 +33,7 @@
 <config>
   <modules>
     <Cm_RedisSession>
-      <active>true</active>
+      <active>false</active>
       <codePool>community</codePool>
     </Cm_RedisSession>
   </modules>

[colinmollenhour]

I added https://github.com/OpenMage/magento-lts/pull/3465 so that v19 users at least have the option of setting 'redis' explicitly rather than relying on the rewrite.

[fballiano]

@colinmollenhour I can reproduce the problem on v20 too so I don't know if #3465 would help

[rvelhote]

I could remove the rewrite again and release it as version 3.2.0 with a note in the README to add the rewrite to local.xml where needed, but non-OpenMage users or non-updated users that don't have the version fixed in composer.json and people using the repo directly would be affected unexpectedly since the rewrite would be missing.

That said, does the db adapter really add any value? For small installations the files adapter is jsut as good or better and for large (multi-node or high-traffic) installations the Redis adapter is much better than the MySQL one. Is there a use-case I'm missing for the 'db' adapter?

@colinmollenhour I am using the DB adapter in a multi-node environment. At this moment, I would rather keep it in the database but am open to try Redis for sessions again in the future. In the past (2017/18?) I ran into some issues - I believe - with locks or connections.

[colinmollenhour]

I see. The db adapter uses zero locking so if you are ok with that it is not a bad choice. However, you can tune the redis adapter so that it is more forgiving like setting max_concurrency=1, breakafter*=1, etc. or just disable locking entirely with disable_locking=1. Aside from the locking it still has a few benefits like compression and fast bot session expiration.

[colinmollenhour]

@colinmollenhour I can reproduce the problem on v20 too so I don't know if #3465 would help

It doesn't really "fix" this issue, the problem is that it seems impossible to fix all environments simultaneously - either we remove the rewrites in Cm_RedisSession or we don't - merging #3465 would just help reduce the impact if we were to remove the rewrites so even if we left it alone for now it would be good to go ahead and merge #3465 so people can start using 'redis' instead of 'db' in their configs.

[fballiano]

but @rvelhot is on v20 so I don't know, should we do that in v19, with all the discussions we had?

[colinmollenhour]

but @rvelhot is on v20 so I don't know, should we do that in v19, with all the discussions we had?

There is zero downside - had we done it a long time ago we could probably remove the rewrite now and just update the README so I thought we should go ahead and get it in there in case there is another v19 release at some point.

[fballiano]

Since it's fixed upstream and it covers the original bug report, I'll close this.