Increase redis session max_concurrency limit from 6 to 32 #29140

marvinhinz · 2020-07-15T07:36:02Z

Description (*)

The default redis session concurrency limit is fairly low. This sometimes triggers error pages for customers who might have opened multiple links in new tabs.

I see no real drawback increasing this default value from 6 to 32.
For us this solved the problem completely in production systems.

Additional investigation is described in #17310 (comment) and #17310 (comment)

Fixed Issues (if relevant)

Fixes session_start(): Failed to read session data: user (path: /var/lib/php/sessions) #17310 that was wrongly closed by a now deleted user.

Manual testing scenarios (*) copied from @chris-pook

Using Google Chrome, login to the admin area
Open 10 or more browser tabs with the orders grid page
Click the left most tab
Hold shift and click the right most tab (this selects all tabs)
Right click the right most tab and select "reload"
Check the tabs after they have all reloaded, only ~6 will successfully load, the rest will exhibit this error *
Check the var/report folder for the start_session error report relating to the tabs that failed.
Now modify the max_concurrency to a value greater than the number of tabs, reload them all again and none will fail.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

The default redis session concurrency limit is fairly low. Every magento page load triggers many ansync requests, combined with moderate traffic this causes frequent errors. I see no real drawback increasing this default value. For us this solved the problem completely in production systems. This fixes issue magento#17310 that was wrongly closed by a now deleted user.

m2-assistant · 2020-07-15T07:36:04Z

Hi @marvinhinz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

marvinhinz · 2020-07-15T08:58:24Z

@magento run all tests

ihor-sviziev · 2020-07-15T09:14:50Z

@marvinhinz this case looks really strange and not sure if we really need to fix it.
Could you describe what the real issue you have on production? I don't think anyone from customers will reload 10 tabs at once. Is there any use case when it's needed?
Also not clear - do we have such issue in case if we use for instance session in files instead of redis? What behavior we have in this case?

marvinhinz · 2020-07-15T11:17:14Z

@marvinhinz this case looks really strange and not sure if we really need to fix it.
Could you describe what the real issue you have on production? I don't think anyone from customers will reload 10 tabs at once. Is there any use case when it's needed?

Thats actually a real issue. When testing our production systems its not uncommon to open many links with the ctrl key pressed, or using the mouswheelbutton. Also personally some customers tend to do this for e.g. comparing products/prices etc.

Also not clear - do we have such issue in case if we use for instance session in files instead of redis? What behavior we have in this case?

No, afaik this happens only with redis session enabled.

ihor-sviziev · 2020-07-15T11:44:52Z

@marvinhinz thank you for the info! will be really useful!

ihor-sviziev

✔ Approved.

Failing tests looks not related to changes form this PR.

magento-engcom-team · 2020-07-17T06:35:49Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7852 has been created to process this Pull Request

fritzmg · 2020-07-17T07:52:46Z

In our instances even 32 was not enough. Why not set it even higher by default? Is there a drawback?

fritzmg · 2020-07-17T07:54:45Z

I don't think anyone from customers will reload 10 tabs at once. Is there any use case when it's needed?

The problems arise because Magento will cause several AJAX requests to be executed after the initial request. E.g. for loading customer sections, or JavaScript modules or any other content, that is loaded asynchronously. And depending on the theme and extensions you have installed, this number may increase drastically.

marvinhinz · 2020-07-17T11:02:54Z

In our instances even 32 was not enough. Why not set it even higher by default? Is there a drawback?

What value was sufficient for your projects? I dont think there would be drawbacks increasing it to for example 128?

@ihor-sviziev what do you think?

ihor-sviziev · 2020-07-17T11:04:22Z

I think it was set to such small value by default in order to prevent single customer to send too much requests and causing big load on production instance

…

On Fri, 17 Jul 2020 at 10:53, Fritz Michael Gschwantner < ***@***.***> wrote: In our instances even 32 was not enough. Why not set it even higher by default? Is there a drawback? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29140 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOJOUNUK7WNWEQOSCBWUETR377N5ANCNFSM4O2IFKFQ> .

fritzmg · 2020-07-17T11:28:31Z

@marvinhinz we used 64 in the end.

@ihor-sviziev but does that make much sense? The requests are not prevented by this. And even if you want to reduce the load on the Redis server this way, then the fallback needs to work, which it currently does not (see #17310). And even if the fallback would work, it would simply increase the I/O load on the server - which is something that using the Redis Session Cache is supposed to avoid.

engcom-Alfa · 2020-07-21T12:08:16Z

✔️ QA Passed

Preconditions:

Redis server;
Use Redis for session storage devdocs

'session' =>
    array (
      'save' => 'redis',
      'redis' =>
      array (
        'host' => '127.0.0.1',
        'port' => '6379',
        'password' => '',
        'timeout' => '2.5',
        'persistent_identifier' => '',
        'database' => '2',
        'compression_threshold' => '2048',
        'compression_library' => 'gzip',
        'log_level' => '4',
        'max_concurrency' => '6',
        'break_after_frontend' => '5',
        'break_after_adminhtml' => '30',
        'first_lifetime' => '600',
        'bot_first_lifetime' => '60',
        'bot_lifetime' => '7200',
        'disable_locking' => '0',
        'min_lifetime' => '60',
        'max_lifetime' => '2592000'
      )
    ),

Manual testing scenario:

Using Google Chrome, login to the admin area;
Open 16 tabs with the orders page for example
Click the left most tab;
Right click the right most tab and select "reload"
Check the tabs after they have all reloaded;

Before: ✖️ Only ~6 will successfully load and others will have errors

Check the var/report folder for the start_session error report relating to the tabs that failed.

After: ✔️ Browsing pages in the admin area didn't throw an error

marvinhinz · 2020-07-24T12:24:14Z

Will this be included in the 2.4 release? :)

fritzmg · 2020-07-24T12:29:28Z

Will this be included in the 2.4 release? :)

This is something you can already do yourself. It does not fix the original issue.

marvinhinz · 2020-07-24T14:36:57Z

Will this be included in the 2.4 release? :)

This is something you can already do yourself. It does not fix the original issue.

I know, but it would be nice if newly installed shops dont run into this issue in the first place.

ihor-sviziev · 2020-08-04T11:48:14Z

I just found that this PR was discussed on the public triage meeting in https://www.youtube.com/watch?v=knegmLW94z4&t=1575s
but it still requires review from dev. side.

@sdzhepa @VladimirZaets just adding my thoughts about this PR and issue itself.

The issue appears not only with requests to the pages itself, but also to AJAX requests on the pages you're just opened opened, so sometimes you don't really need to open 6 pages, but just 2-3.

Real use case when this issue reproducing:
For marketing manager this is quite typical situation when you need to review multiple orders, you're opening few orders in a row, and it fails. You have to refresh the pages that failed.

Another use case - on frontend some customers opening few products in a new tab (usually from desktop) people doing that.

About limiting session concurrency - sessions is not the thing for decreasing load on the servers, we should have this limitation on the nginx/phpfpm or even on the WAF level.

About this change - it will change only default settings in env.php during magento installation, people still could increase / decrease this value in env.php file:

ihor-sviziev · 2020-08-18T06:48:15Z

@marvinhinz just an idea - what if we'll implement postponed sending of AJAX calls while tab is not active? As result - we'll send less requests in such cases to the backend and this issue probably will be also much less reproducible. We can use for that https://developer.mozilla.org/en-US/docs/Web/API/Page_Visibility_API

Nazar65 · 2020-10-15T10:45:04Z

@ihor-sviziev just in case -> https://xumulus.com/magento-2-and-ajax-with-optimistic-session-locking/

I am wondering if it would make sense to implement an option (set through a special parameter using define() maybe?) that would change the session to read-only mode (this would mean disable locking on read and do nothing on write).

Nazar65 · 2020-10-15T10:47:05Z

@ihor-sviziev this pr will not resolve the issue, with 32 number
also may be related -> #14973

ihor-sviziev

Hi @marvinhinz,
As we discussed with @Nazar65 - this PR actually masks the issue, instead of fixing it.

Proper solution will be introducing optimistic session locking mechanism, as described here #29140 (comment). That's definitely will be much bigger effort than this PR.

Will you be able to update your PR by introducing optimistic session locking mechanism?

ihor-sviziev · 2020-10-29T15:57:30Z

@marvinhinz I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

Note: @Nazar65 you can take this issue

m2-assistant · 2020-10-29T15:57:32Z

Hi @marvinhinz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

ghost added the Progress: pending review label Jul 15, 2020

magento-engcom-team added Component: Setup Release Line: 2.4 labels Jul 15, 2020

marvinhinz changed the title ~~Update Session.php~~ Increase redis session max_concurrency limit Jul 15, 2020

marvinhinz changed the title ~~Increase redis session max_concurrency limit~~ Increase redis session max_concurrency limit from 6 to 32 Jul 15, 2020

ihor-sviziev self-assigned this Jul 15, 2020

ihor-sviziev added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests improvement labels Jul 15, 2020

ihor-sviziev mentioned this pull request Jul 17, 2020

session_start(): Failed to read session data: user (path: /var/lib/php/sessions) #17310

Open

ghost added the Severity: S1 Affects critical data or functionality and forces users to employ a workaround. label Jul 17, 2020

ihor-sviziev added the Award: bug fix label Jul 17, 2020

ihor-sviziev approved these changes Jul 17, 2020

View reviewed changes

ghost added Progress: ready for testing and removed Progress: pending review labels Jul 17, 2020

engcom-Alfa self-assigned this Jul 21, 2020

ghost added the Progress: testing in progress label Jul 21, 2020

ghost added Progress: extended testing and removed Progress: testing in progress labels Jul 21, 2020

engcom-Echo self-assigned this Jul 21, 2020

engcom-Echo added the QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) label Jul 21, 2020

ghost added Progress: ready for testing and removed Progress: extended testing labels Aug 27, 2020

engcom-Echo removed their assignment Oct 2, 2020

ihor-sviziev requested changes Oct 15, 2020

View reviewed changes

ghost added Progress: needs update and removed Progress: ready for testing labels Oct 15, 2020

ihor-sviziev closed this Oct 29, 2020

ghost removed the Progress: needs update label Oct 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase redis session max_concurrency limit from 6 to 32 #29140

Increase redis session max_concurrency limit from 6 to 32 #29140

marvinhinz commented Jul 15, 2020 •

edited by ihor-sviziev

Loading

m2-assistant bot commented Jul 15, 2020

marvinhinz commented Jul 15, 2020

ihor-sviziev commented Jul 15, 2020 •

edited

Loading

marvinhinz commented Jul 15, 2020

ihor-sviziev commented Jul 15, 2020

ihor-sviziev left a comment

magento-engcom-team commented Jul 17, 2020

fritzmg commented Jul 17, 2020

fritzmg commented Jul 17, 2020 •

edited

Loading

marvinhinz commented Jul 17, 2020

ihor-sviziev commented Jul 17, 2020 via email

fritzmg commented Jul 17, 2020 •

edited

Loading

engcom-Alfa commented Jul 21, 2020

marvinhinz commented Jul 24, 2020

fritzmg commented Jul 24, 2020

marvinhinz commented Jul 24, 2020

ihor-sviziev commented Aug 4, 2020 •

edited

Loading

ihor-sviziev commented Aug 18, 2020

Nazar65 commented Oct 15, 2020 •

edited

Loading

Nazar65 commented Oct 15, 2020 •

edited

Loading

ihor-sviziev left a comment

ihor-sviziev commented Oct 29, 2020

m2-assistant bot commented Oct 29, 2020

Increase redis session max_concurrency limit from 6 to 32 #29140

Increase redis session max_concurrency limit from 6 to 32 #29140

Conversation

marvinhinz commented Jul 15, 2020 • edited by ihor-sviziev Loading

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*) copied from @chris-pook

Contribution checklist (*)

m2-assistant bot commented Jul 15, 2020

marvinhinz commented Jul 15, 2020

ihor-sviziev commented Jul 15, 2020 • edited Loading

marvinhinz commented Jul 15, 2020

ihor-sviziev commented Jul 15, 2020

ihor-sviziev left a comment

Choose a reason for hiding this comment

magento-engcom-team commented Jul 17, 2020

fritzmg commented Jul 17, 2020

fritzmg commented Jul 17, 2020 • edited Loading

marvinhinz commented Jul 17, 2020

ihor-sviziev commented Jul 17, 2020 via email

fritzmg commented Jul 17, 2020 • edited Loading

engcom-Alfa commented Jul 21, 2020

marvinhinz commented Jul 24, 2020

fritzmg commented Jul 24, 2020

marvinhinz commented Jul 24, 2020

ihor-sviziev commented Aug 4, 2020 • edited Loading

ihor-sviziev commented Aug 18, 2020

Nazar65 commented Oct 15, 2020 • edited Loading

Nazar65 commented Oct 15, 2020 • edited Loading

ihor-sviziev left a comment

Choose a reason for hiding this comment

ihor-sviziev commented Oct 29, 2020

m2-assistant bot commented Oct 29, 2020

marvinhinz commented Jul 15, 2020 •

edited by ihor-sviziev

Loading

ihor-sviziev commented Jul 15, 2020 •

edited

Loading

fritzmg commented Jul 17, 2020 •

edited

Loading

fritzmg commented Jul 17, 2020 •

edited

Loading

ihor-sviziev commented Aug 4, 2020 •

edited

Loading

Nazar65 commented Oct 15, 2020 •

edited

Loading

Nazar65 commented Oct 15, 2020 •

edited

Loading