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

Jump to bottom

Users can get logged out when many notifications are pending #17732

Closed
tsteur opened this issue Jul 1, 2021 · 3 comments · Fixed by #17736
Closed

Users can get logged out when many notifications are pending #17732

tsteur opened this issue Jul 1, 2021 · 3 comments · Fixed by #17736
Assignees
@diosmosis
Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone
4.5.0

Comments

@tsteur
Copy link
Member

tsteur commented Jul 1, 2021

You can reproduce it this way:

It doesn't happen when I apply below change:

diff --git a/core/Notification/Manager.php b/core/Notification/Manager.php
index a621f27bfd..789e71a7c3 100644
--- a/core/Notification/Manager.php
+++ b/core/Notification/Manager.php
@@ -182,6 +182,7 @@ class Manager
 
     private static function isSessionEnabled()
     {
+        return false;
         return Session::isWritable() && Session::isReadable();
     }

Background: The requests on this page currently trigger notifications Trying to add two strings in DataTable\Row::sumRowArray... this is why this can't be reproduced on other cases.

I'm thinking this might be a regression from either https://github.com/matomo-org/matomo/pull/17413/files#diff-45b10ec69da2f6ebcecbfef283c2a608427a37f0f4790d0813fc4892894247d5R773 or maybe ba6be40#diff-943fba627b25c62a3fb10c11ee813fce6f225f90f85d1317f05ab07bbfba3e52R524 or some other change. Possible though that this has been an issue for longer and only happens when a big number of notifications are stored or so.
@tsteur tsteur added Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Jul 1, 2021
@tsteur tsteur added this to the 4.4.0 milestone Jul 1, 2021
@diosmosis diosmosis self-assigned this Jul 4, 2021
@diosmosis
Copy link
Member

Some things I noticed:

  • the session data simply doesn't exist when the error occurs (it's not in the database, and $_SESSION is empty)
  • before the row evolution popover is opened, the session will exist in the DB
  • after the row evolution popover shows up, one or more of the requests triggers the destruction of the session. I'm not sure if it's done in PHP or some other strange error

@diosmosis
Copy link
Member

More things:

  • the session is destroyed after it is failed to be read
  • it fails to read because the length of the data is likely cut off, either when it is saved or read. I could only see it try to read 65535, not write it. This is the max length of a TEXT column, so the serialize would fail causing the session to be incorrectly read.

@diosmosis diosmosis mentioned this issue Jul 5, 2021
Merged
10 tasks
@tsteur
Copy link
Member Author

tsteur commented Jul 8, 2021

just fyi I fixed the bug that causes the notices to happen in the first place on demo2 (in the plugin). should we need to reproduce the issue there again let me know and I can apply the bug again.

@mattab mattab modified the milestones: 4.4.0, 4.5.0 Jul 28, 2021
@justinvelluppillai justinvelluppillai changed the title Getting logged out when notifications are persisted in storage Users can get logged out when many notifications are pending Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@diosmosis diosmosis

Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Milestone
4.5.0
Development

Successfully merging a pull request may close this issue.

avoid large amounts of notifications being added to the session matomo-org/matomo
3 participants
@diosmosis @tsteur @mattab