? Pending

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
14 Feb 2018

Because apparently nobody seems to care that garbage collection doesn't happen until it hits the database, put back the broken session management code so that the database stays happy.

avatar mbabker mbabker - open - 14 Feb 2018
avatar mbabker mbabker - change - 14 Feb 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 14 Feb 2018
Category Libraries
avatar brianteeman
brianteeman - comment - 14 Feb 2018

It is NOT true that nobody cares!!!!

avatar csthomas
csthomas - comment - 14 Feb 2018

This PR is not a solution for me ?

Cronjob is designed to do that. For other users that does not have that possibility, they should use a plugin designed for that (for B/C it should be enabled).

avatar joomdonation
joomdonation - comment - 14 Feb 2018

Could we add a new config option (show-on option, only show when Database Handler is used):

Delete session on page load with two options Yes/No. Yes should be default behavior and for power users, he can set it to No and setup cron job merged in PR #19548 to do the cleanup?

avatar mbabker
mbabker - comment - 14 Feb 2018

No. We should not have a different behavior path for the database handler in relation to other handlers.

avatar joomdonation
joomdonation - comment - 14 Feb 2018

To be fair, the session data which we are deleting on page load at the moment can be considered as session meta data. Even we can move it to be stored in a separate table as you mentioned elsewhere, that delete query should be run on page load (at least default behavior) because look like it is difficult for end users to setup cron job to do the clean up

avatar mbabker
mbabker - comment - 14 Feb 2018

The point in moving the cleanup operation to the end of the request cycle was to move the performance penalty for that operation from being a blocking event to something that happens after the user should have the response document so the penalty (even if we're only talking about milliseconds) isn't seen by the user as visibly (you might still see it in a network waterfall chart on dev tools).

avatar mbabker
mbabker - comment - 14 Feb 2018

Additionally, 3.8.3 and earlier don't error trap that operation. So if that query fails for whatever reason, it results in a 500 response. There is no reason that needs to be a request blocking operation, and no reason the cleanup operation needs to be a pre-execute action versus something that can happen post-execute.

avatar joomdonation
joomdonation - comment - 14 Feb 2018

The point in moving the cleanup operation to the end of the request cycle

I don't think anyone here against that change. That's why people don't like this full revert (I even prepared a PR to do partial revert https://github.com/joomla/joomla-cms/compare/staging...joomdonation:patch-20?expand=1 but didn't submit it)

The point here is that we need to clean up session meta data somehow and the easiest way to do that is through page load as how we were doing. Adding a new setting to allow disable that clean up on page load for power users as I said should make everyone happy (again, this is not related to session session garbage collection, it is just the session meta data)

avatar mbabker
mbabker - comment - 14 Feb 2018

The cleanup is still happening during a HTTP request cycle. Just at the end of it, not at the beginning. As it should be, this maintenance task should not be a request blocking task and having it at the beginning as was the case in 3.8.3 and earlier makes it request blocking.

I've been fighting for over two years to fix every architectural flaw in the session API (both with the real data and the metadata). Every pull request I have made to fix that gets reverted for one reason or another, and I honestly don't have the energy anymore to keep fighting a broken community that is willing to accept that a critical API piece is fatally implemented. I've laid out exactly what I am trying to do, to make things manageable for both "simple" users who just want Joomla to run and advanced users who are able to move these maintenance tasks out of the HTTP request scope (i.e. cron job). But aside from you and maybe 3 or 4 others, it is next to impossible to convince people that the architectural benefits are worth the growing pains.

avatar joomdonation
joomdonation - comment - 14 Feb 2018

The cleanup is still happening during a HTTP request cycle. Just at the end of it, not at the beginning

Except that it is not happening for Database Session Handler and that's why people complain about it

If it is possible, I would suggest that:

  1. Restore the delete session meta data on page load for Database Handler
  2. Continue working to improve session as you described for 3.9 (please re-open the PRs which you closed).
avatar mbabker
mbabker - comment - 14 Feb 2018

Restore the delete session meta data on page load for Database Handler

I'm trying not to, because at that point we are back to the application class acting as an arbitrary session garbage collector for the database handler.

As explained in #19585 (comment) I will prepare a PR that will have everything needed to make the system work efficiently. The one hard line I am going to draw in the sand though is that I am not removing the database check. The code that I am writing/proposing explicitly differentiates between the metadata and the session data, and explicitly only cleans the metadata (and as explained, with the schema changes I am aiming for, in 4.0 that check will be unnecessary).

avatar joomdonation
joomdonation - comment - 14 Feb 2018

I'm trying not to, because at that point we are back to the application class acting as an arbitrary session garbage collector for the database handler.

As mentioned, the intention is clear session meta data. We just have that issue because we store both session and session meta data in the same table which we cannot address until J4 because B/C promise. Assume we move session meta data to a separate table In J4, we need that delete operation anyway

Anyway, looking forward to seeing that PR. And please close this un-welcomed PR.

avatar Gitjk
Gitjk - comment - 15 Feb 2018

I've been fighting for over two years to fix every architectural flaw in the session API

Reminds me of 'Hackwar's' fight to fix the router (7 years). Sometimes contributors to Joomla need extraordinary perseverance. :-)

avatar brianteeman
brianteeman - comment - 20 Feb 2018

Closed as we have #19687 Please test it!!

avatar brianteeman brianteeman - close - 20 Feb 2018
avatar brianteeman brianteeman - change - 20 Feb 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-02-20 08:47:04
Closed_By brianteeman
Labels Added: ?

Add a Comment

Login with GitHub to post a comment