Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concurrent (quick reload) requests on checkout cause cart to empty - related to session_regenerate_id #12362

Closed
minlare opened this issue Nov 20, 2017 · 74 comments
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Comments

@minlare
Copy link

minlare commented Nov 20, 2017

Preconditions

  1. Magento 2.2.0
  2. PHP7

Steps to reproduce

  1. Add product to cart
  2. Navigate to checkout
  3. Reload the page as quick as possible (F5 multiple times)

Expected result

  1. Reload the checkout as usual
  2. Cart contains session items

Actual result

  1. Redirects to empty cart
  2. session_id() is old session id (previous request regenerated a new session_id, but this request has old session_id)

http://php.net/manual/en/function.session-regenerate-id.php

In https://github.com/magento/magento2/blob/2.2-develop/lib/internal/Magento/Framework/Session/SessionManager.php#L507 you reference http://php.net/manual/en/function.session-regenerate-id.php#53480 however I believe on concurrent requests this does not work correctly...

@magento-engcom-team magento-engcom-team added Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Cannot Reproduce Cannot reproduce the issue on the latest `2.4-develop` branch labels Nov 20, 2017
@magento-engcom-team magento-engcom-team removed the Issue: Cannot Reproduce Cannot reproduce the issue on the latest `2.4-develop` branch label Nov 20, 2017
@minlare
Copy link
Author

minlare commented Nov 20, 2017

I see you have re-opened the issue yourselves... Does that mean you were able to reproduce?

@magento-engcom-team magento-engcom-team added the Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed label Nov 20, 2017
@magento-engcom-team
Copy link
Contributor

@minlare, thank you for your report.
We've created internal ticket(s) MAGETWO-84160 to track progress on the issue.

@magento-engcom-team magento-engcom-team added 2.1.x Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 20, 2017
@gewaechshaus
Copy link

gewaechshaus commented Nov 20, 2017

Very good finding @minlare - Respect, this is a real conversion killer. Everybody can test it in one of our m2.2.1 live systems on https://www.makerdise.com... It's easy to reproduce.

@minlare - are you using varnish?

@josh-nh
Copy link

josh-nh commented Nov 20, 2017

Following..

@minlare
Copy link
Author

minlare commented Nov 21, 2017

Thanks @gewaechshaus - it was a bit of a shock to find no-one else had raised the issue! I just tested on a base install of Magento 2.1.9 & 2.2.0 with sample data, so you'd have to test with varnish yourself...

@southerncomputer
Copy link
Contributor

i've seen this on 2.1.10 with classylama_defaultbillingaddress plugin with custom OPC! It causes billing-information to loop 4 times - which then looses the session cookie!

@gewaechshaus
Copy link

gewaechshaus commented Nov 21, 2017

@southerncomputer - we are also using a custom OPC. But you can test the regular checkout by just attaching /checkout/ instead of /onestepcheckout/ to the url - it does behave the same way.

However, I did test it on E3D website which doesn't seem to have this bug, but is also using a custom checkout.
https://e3d-online.com/

You can also easily reproduce this in
http://demo-acm2.bird.eu
https://magento.nublue.co.uk

@magento-engcom-team - This bug should be handled with the highest possible priority!

@acetronaut
Copy link

I have implemented some changes that seem to resolve the issue but if anyone can look over this and feedback that would be great

vendor/magento/framework/Session/SessionManager.php

The regenerateId function needs replacing with this

    public function regenerateId()
    {
        if (headers_sent()) {
            return $this;
        }

        if ($this->isSessionExists()) {
            $oldSessionId = session_id();            
            session_regenerate_id();   //regen the session
            $new_session_id = session_id();
                    
            $_SESSION['new_session_id'] = $new_session_id;
            
            // Set destroy timestamp
            $_SESSION['destroyed'] = time();
            
            // Write and close current session;
            session_commit();
            $oldSession = $_SESSION;   //called after destroy - see destroy!
            // Start session with new session ID
            session_id($new_session_id);
            ini_set('session.use_strict_mode', 0);
            session_start();
            ini_set('session.use_strict_mode', 1);
            $_SESSION = $oldSession;
            // New session does not need them
            unset($_SESSION['destroyed']);
            unset($_SESSION['new_session_id']);  
        } else {
            session_start();
        }
        $this->storage->init(isset($_SESSION) ? $_SESSION : []);

        if ($this->sessionConfig->getUseCookies()) {
            $this->clearSubDomainSessionCookie();
        }
        return $this;
    }

and also the start function needs replacing with this

    public function start()
    {
        if (!$this->isSessionExists()) {
            \Magento\Framework\Profiler::start('session_start');

            try {
                $this->appState->getAreaCode();
            } catch (\Magento\Framework\Exception\LocalizedException $e) {
                throw new \Magento\Framework\Exception\SessionException(
                    new \Magento\Framework\Phrase(
                        'Area code not set: Area code must be set before starting a session.'
                    ),
                    $e
                );
            }

            // Need to apply the config options so they can be ready by session_start
            $this->initIniOptions();
            $this->registerSaveHandler();
            if (isset($_SESSION['new_session_id'])) {
             // Not fully expired yet. Could be lost cookie by unstable network.
             session_commit();
             session_id($_SESSION['new_session_id']);
             }
            // potential custom logic for session id (ex. switching between hosts)
            $this->setSessionId($this->sidResolver->getSid($this));
            session_start();
            if (isset($_SESSION['destroyed'])) {
               if ($_SESSION['destroyed'] < time()-300) {
                   $this->destroy(['clear_storage' => true]);
                   
               }
            }
            $this->validator->validate($this);

            register_shutdown_function([$this, 'writeClose']);

            $this->_addHost();
            \Magento\Framework\Profiler::stop('session_start');
        }
        $this->storage->init(isset($_SESSION) ? $_SESSION : []);
        return $this;
    }

@gewaechshaus
Copy link

@acetronaut - give us a few hours...

@gewaechshaus
Copy link

gewaechshaus commented Nov 22, 2017

@acetronaut

You got it

@acetronaut
Copy link

acetronaut commented Nov 22, 2017

Further notes R.E this issue

public function regenerateId() was updated in 2.1.9 and most likely the reason this isnt apparent on all M2 instances

https://github.com/magento/magento2/blob/2.1.8/lib/internal/Magento/Framework/Session/SessionManager.php

https://github.com/magento/magento2/blob/2.1.9/lib/internal/Magento/Framework/Session/SessionManager.php

So a revert to the previous code may resolve the issue - however possibly re-introduce a different bug this was changing anyway

If anyone knows why this was changed & what bug it was meant to fix, please let me know!

@southerncomputer
Copy link
Contributor

The above patch does not work. you guys have redis locking on sessions enabled?? there was a time when the config read locking inverted!!

@acetronaut
Copy link

its not a redis issue - you can disable redis and replicate the same thing

however the problem does kind of get worse - because session_regenerate_id actually calls the read function in redis as well as the session start

#12362
This is a change to how PHP 7 works over 5.6

The above fix works - but im trying to confirm with magento status on their internal ticket for looking at this issue with more detail, ill be chasing them hard on this one

so basically if you use M2.1.9+ and the redis inverted fix to turn locking on, it causes a loop in the locking function

@keithbentrup identified the issue on the admin section - but this stretches much deeper as the checkout calls regenerateId and adds a lot of overhead onto the checkout process if you have locking turned on, enough to kill your conversions and cause customer dropouts anyway

so you kind of have to weigh up i guess whether instead of updating the function above, whether you should leave the redis locking fix in place for the benefits this will give you and remove the call to $this->_customerSession->regenerateId(); in this file

/vendor/magento/module-checkout/Controller/Index/Index.php

im trying at the moment to justify any security issues that could arise from this by session hijacking, especially as the regenerateId is also called on the login function itself

there are a few seperate but major issues at play here, ill feedback what i can on this, but at the same time if anyone else is debugging either of these issues in detail, please feel free to share your thoughts!

@southerncomputer
Copy link
Contributor

Why would you call session regenerate from there? There a recent patch I applied that extends sessions on frontend to not let them timeout.

I'm getting a bug in my OPC where multiple dispatches starting at xhr billing-address are being called 3-simultaneosly that starts this issue!

@acetronaut
Copy link

you would have to ask magento for more details on why specifically they are calling it from there, but most likely its to prevent CSRF / XSS attacks, but i think its also called on login too anyway.

it bugs me a little anyway if there is a legitimate reason for it being there anyway such as possible session hijacking - that there isnt a note linking to doc that explains this, just so people dont remove it from the code

but anyway this wouldnt cause an issue anyway if the regenerate function wasnt causing the basket to drop, and also wouldnt cause an issue if the redis module didnt have the bug causing the session locking and overhead

these issues above affect core behaviour of magento, so it would be worth you stripping back, removing any modules or changes, replicating the core issues, fixing the core issues and building up from there so you can isolate if your issues are either the ones described here or new ones outside the scope of this problem

@southerncomputer
Copy link
Contributor

Since the session expiration is only set on login/checkout - i suspect the regenerate is there to make sure you don't timeout during checkout!

@erikhansen
Copy link
Contributor

Here is a patch that should fix the issue, until 2.2.7 is released. This patch contains the relevant contents of the f82a17 commit referenced in the comment from May 12th.

github-12362-composer.patch.zip

@ishakhsuvarov
Copy link
Contributor

@congson95dev
Copy link

Quick fixed:
https://magento.stackexchange.com/a/246493/70258
Hope it help some one.
Thanks :)

@gabrieljadeau
Copy link

hi @saxsax1995 : your patch isn't working for m2.2.5. Do you have link for this one ?

Best,

@congson95dev
Copy link

hi @gabrieljadeau , maybe you will have to custom by yourself, that's what i did back there :( because each magento version is difference :(

@winterk80
Copy link

Confirmed this is occurring in m2.2.5 EE

@tuyennn
Copy link
Member

tuyennn commented Dec 14, 2018

Same issue on server running redis and magento 2.1.9

#15641 #13427

@Alexander-Pop
Copy link

@hmphu
Copy link

hmphu commented Jul 22, 2019

@Alexander-Pop have you try this magento2-session-unblocker module? Is it good

@Alexander-Pop
Copy link

@Alexander-Pop have you try this magento2-session-unblocker module? Is it good

Nope. Asked myself

@aleksejtsebinoga
Copy link

@hmphu @Alexander-Pop @winterk80 @tuyennn @gabrieljadeau ,

Guys, you can try this composer package, which is wrapping all the knowledge found on the web.
It was originally developed by me for Scandiweb, and now it became public -

https://bitbucket.org/scandiweb/sessionfix/src/master/

It would be great to know if it worked for you.

@hmphu
Copy link

hmphu commented Jul 24, 2019

Thanks @aleksejtsebinoga I will try it

@marco7319
Copy link

marco7319 commented Jul 27, 2019

@aleksejtsebinoga

Hello,
can you explain to me in detail the steps please?
I use magento 2.3.2
cart empty when i add product in the cart

thanks

aleksejtsebinoga> @hmphu @Alexander-Pop @winterk80 @tuyennn @gabrieljadeau ,

Guys, you can try this composer package, which is wrapping all the knowledge found on the web.
It was originally developed by me for Scandiweb, and now it became public -

https://bitbucket.org/scandiweb/sessionfix/src/master/
#aleksejtsebinoga
It would be great to know if it worked for you.

@hammockvienna
Copy link

i can confirm report of @marco7319 , i have the same issue on 2.3.2.

When i add something to the cart, the cart is not empty from a frontend user experience. BUT the small cart icon does not show (1). I have also seen occurrences that the small icon had (1) but after the page completed loading the (1) was gone. When i proceed to checkout the cart is already empty.
In the bug description above it only mentions that the issue happens on the checkout page with multiple reloading.

I also have seen that we dont have that issue on an testenvironment where is no traffic. The issue appears only in production with traffic? Is that logical?
We even dont reach the checkout page.
@aleksejtsebinoga : Could the reason be the same like the description above (race conditions?)
@hmphu did you try it? Did it solve the issue?

thanx

@aleksejtsebinoga
Copy link

@hammockvienna, @marco7319

Ah, sorry. I should've mentioned, that the patch was developed for 2.2.x.
Never tested it with 2.3.x.

But I suppose that they've updated session management. So it is not compatible anymore.

But you still have the empty cart problem in 2.3.2 without the patch?

@hammockvienna
Copy link

@aleksejtsebinoga
yes i have that issue as discribed above. The product is in the cart, but the cart i can changes after some ajax requests from (1) to (0). After that if i reload, the product is gone.
In Magento 2.2.8 i didnt have that issue, but we would like to upgrade to 2.3.2 and there we have it :-(

@Alexander-Pop which Magento version do you use? And did you check the magento2-session-unblocker in the meantime?

@marco7319
Copy link

marco7319 commented Jul 29, 2019

see ticket #23923 . Is it an operating system problem? to install linux on his computer? or have a linux server?

@hammockvienna
Copy link

@marco7319 Its a Vultr linux server (In #23923 they mention windows)

@erfanimani
Copy link
Contributor

For everyone that is experiencing empty cart issues, a good thing to check is to see if your session is actually different. So compare the PHPSESSID cookie value before and after the cart becomes empty. If it's the same session ID, it might not actually be a session loss issue, but something else that messes up your cart.

@aleksejtsebinoga
Copy link

@erfanimani , @marco7319 , @hammockvienna ,

Guys, please note that this thread is for concurrent session issue in checkout.
This bug happens, when you are landing into the checkout when it is under high load.

It is not related to the empty cart on the product adding.

@Alexander-Pop
Copy link

@hammockvienna still on M1 having all issues described here. In process of migration to M2 hopping that session problem will go away but as i see this "ghost" still appear from time to time on some stores...

@wajahatbashir
Copy link

I'm facing same issue on my live server, when i'm adding product to cart the ajax requests from (1) to (0). i'm getting this message (Product added successfully) But mini cart, cart page and checkout are empty. I'm using Magento 2.3.0.

On localhost add to cart is working fine but on live server this is not working.

@pmathbliss
Copy link

For me, there was an error tangent to this with the wishlist section had products that no longer exists.

Try changing the Magento\Customer\Controller\Section\Load:execute()
response
from
$response = ['message' => $this->getEscaper()->escapeHtml($e->getMessage())];
to
$response = ['message' => $this->getEscaper()->escapeHtml($e->getTraceAsString())];

then reload the frontend to see the stack trace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

No branches or pull requests