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

fix: Remove duplicate memory session creation on cron jobs #38722

Closed
wants to merge 1 commit into from

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jun 9, 2023

Summary

There might be scenarios (especially on previous versions where session writes do not reopen) where application code lie the encryption KeyManager still holds the previously initialized Memory session which they obtained through DI. There should be no need to close the existing one and create a new one and crypto wrappers should also be unneeded as we are storing in memory only anyways.

There is already a memory session by default which is available if no session is set manually through OC::initSession or the calls removed in ths PR.

$session = new \OC\Session\Memory('');

TODO

  • Waiting for CI

Checklist

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr added bug 2. developing Work in progress labels Jun 9, 2023
@juliusknorr juliusknorr added this to the Nextcloud 28 milestone Jun 9, 2023
@juliusknorr juliusknorr requested review from a team, ArtificialOwl, icewind1991 and nfebe and removed request for a team June 9, 2023 12:49
@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 9, 2023
Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

@juliusknorr juliusknorr marked this pull request as ready for review June 9, 2023 15:29
@juliusknorr juliusknorr requested a review from come-nc October 10, 2023 06:24
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not grasp what this is about…
git says @nickvergessen touched these lines, do you know more?

@nickvergessen
Copy link
Member

PR was owncloud/core#18482
🤔

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@icewind1991
Copy link
Member

icewind1991 commented Feb 9, 2024

Would it make more sense to move the explicit memory session creation to before the app loading?

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 24, 2024
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@juliusknorr
Copy link
Member Author

Closing as I cannot reproduce the underlying issue anymore.

@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@skjnldsv skjnldsv deleted the fix/session-cron branch December 19, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants