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

Revert PR 19165 #19678

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@mbabker
Copy link
Member

commented Feb 14, 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.

Revert "Decrease the frequency of session metadata cleanup and where …
…in the request cycle it occurs (#19165)"

This reverts commit b9abdd1.
@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2018

It is NOT true that nobody cares!!!!

@csthomas

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 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).

@joomdonation

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 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?

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2018

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

@joomdonation

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 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

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 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).

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 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.

@joomdonation

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 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)

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 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.

@joomdonation

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 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).
@mbabker

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 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).

@joomdonation

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 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.

@Gitjk

This comment has been minimized.

Copy link

commented Feb 15, 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. :-)

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

Closed as we have #19687 Please test it!!

@mbabker mbabker deleted the revert-19165 branch Feb 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.