Fix unstable session manager #14973
Fix unstable session manager #14973
Conversation
|
There are a couple of fixes referenced in #12362, some also for 2.2. Have you seen those? |
|
Hi @miguelbalparda not yet, I'll have a deeper look next week. Thanks. |
|
Hi @elioermini , thank you for collaboration. |
|
@elioermini thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository. |
|
Hi @elioermini & @magento-engcom-team @elioermini Did you make any changes / enhancements in this pull request on the code that i wrote here? #12362 There have been reports i can see of a possible issue with http to https, although i think people should ideally have sitewide SSL, i think it will need some testing to ensure there is no issue here. @magento-engcom-team - I wrote this code back in november. I have also got this deployed on several high turnover sites - both CE and EE versions and this seems to be running for us with no issue. Clients are no longer complaining - and we have done some extensive hotjar sessions where we can see this issue no longer present for customers, as well as it increasing conversion / reducing dropouts, especially on mobile One thing i am interested to know though - is we are nearly 6 months after this issue. I can see this issue present on some EE websites online now, i am curious to why there seems to have been lack of acknowlegement on the scale of the issue, and also why the issue seems to have been added to your milestone and then subsequently removed. Further to this public function regenerateId() was updated in 2.1.9 and most likely the reason this isnt apparent on all M2 instances So a revert to the previous code may resolve the issue - however possibly re-introduce a different bug this was changing anyway. Could you let me know and give me a test case please for what issue you were trying to solve with this update which introduced this issue and why you chose not to roll it back. The change you made in 2.1.9 was probably for a reason, and if you have a example of a problem the change you made was solving - i would advise that this pull request also needs retesting against this. Also have noticed some changes a while ago and attempts to make changes, as well as closing some other tickets off in favour of a "Magento Solution" so i am curious whether you can give the magento take on this issue and what has been looked into? It took me 2 solid (and very long) days to write and test this and i am sure you have some very clever people in there that you could assign this to who should be able to look at this in full and also in respect to scenarios that i wont have tested (for example http to https as all of our clients are now 100% SSL). As soon as i became aware of this issue it became a showstopper / top priority for us due to its nature of affecting sales. I was hoping to see some progress from your team by now on the issue. Maybe there has been, and if there has could you please make this public. I hope you decide to engage a little more on the issue with some technical / developer feedback from the changes i made at least. im sure if you had have done this already a lot of clever people in the community could have helped chip in to solve this issue with you I look forward to your reply @magento-engcom-team |
|
Subscribed |
|
Hi @acetronaut yes that's basically the code you added in the comment plus this line to renew the cookie https://github.com/elioermini/magento2/blob/6ee737bb43adcd9b40c170170c5ea8e5d5ad6731/lib/internal/Magento/Framework/Session/SessionManager.php#L201 Thanks a lot for your contribution, it appears to be close to what is being suggested on php.net http://php.net/manual/en/function.session-regenerate-id.php. |
|
Recent internal PR https://github.com/magento/magento2ce/pull/2496 resolves the issues for https only by removing regenerate but this PR should resolve it for mixed mode (http/https) when regenerate is still security best practice. |
|
Hi @elioermini @acetronaut @piotrekkaminski Similar to provided in this Pull Request fix has been evaluated by the core team when investigating this issue. Unfortunately, in some scenarios, specifically when using Redis for session storage it may cause significant performance degradation on some scenarios. @elioermini Have you tried this commit with Redis session storage? Current solution, which is already available in We are going to check the specific solution provided in this Pull Request with possible redis issues in mind and let you know the result. Thank you for collaboration. |
| /** | ||
| * Session destroyed threshold in seconds | ||
| */ | ||
| const SESSION_DESTROYED_THRESHOLD = 300; |
There was a problem hiding this comment.
Please, remove constant and use session lifetime value from the system configuration
| @@ -501,7 +516,29 @@ public function regenerateId() | |||
| return $this; | |||
| } | |||
|
|
|||
| $this->isSessionExists() ? session_regenerate_id(true) : session_start(); | |||
| // @codingStandardsIgnoreStart | |||
There was a problem hiding this comment.
Why was this code fragment suppressed?
There was a problem hiding this comment.
It has been replaced with the if / else block till line 540
| $sid = $this->sidResolver->getSid($this); | ||
| // potential custom logic for session id (ex. switching between hosts) | ||
| $this->setSessionId($sid); | ||
| session_start(); | ||
| if (isset($_SESSION['destroyed'])) { | ||
| if ($_SESSION['destroyed'] < time() - self::SESSION_DESTROYED_THRESHOLD) { |
There was a problem hiding this comment.
Combine this expression with previous if statement
|
@acetronaut That was a really good find on the change to regenerateId(), and I agree with you, that this is most likely the change that broke it. |
|
@elioermini , any updates? |
|
|
|
Hi @elioermini , I fixed minor issues. Please, sign CLA, otherwise, we can't process your pull request |
|
Sorry the delay I got side-tracked, can someone help me making the hardcoded values configurable? Thanks |
|
@elioermini , I think we are done with all the requested changes. The solution looks good to me |
|
Hi @sidolov, thank you for the review. |
|
Hi @elioermini. Thank you for your contribution. Please, consider to port this solution to 2.3 release line. |
ghost
mentioned this pull request
Description
Fix of the unstable behaviour of the session management with high volume of concurrent sessions.
Fixed Issues
Manual testing scenarios
We already deployed the fix on our high traffic website and the issue is fixed.