Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Evaluate PHP's session read_and_close option #29356

Closed
ChristophWurst opened this issue Oct 21, 2021 · 2 comments
Closed

Evaluate PHP's session read_and_close option #29356

ChristophWurst opened this issue Oct 21, 2021 · 2 comments

Comments

@ChristophWurst
Copy link
Member

How to use GitHub

  • Please use the 👍 reaction to show that you are interested into the same feature.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Is your feature request related to a problem? Please describe.

While debugging some concurrency bugs I noticed that requests from a browser pile up in some scenarios. It seems to be caused by PHP's session locking. This might also have an impact on Nextcloud's overall performance, given that we have lots of pages where we sends a high number of concurrent requests (e.g. for avatars in Talk).

Describe the solution you'd like

Evaluate if the suggestions in https://ma.ttias.be/php-session-locking-prevent-sessions-blocking-in-requests/ can be applied for Nextcloud. I gave it a quick tests, it freed up some of the locking and a few more requests could be processed at a time. I used xdebug to pause the requests intentionally, but a well placed sleep(10); achieves the same effect.

\OC\Session\Internal::startSession has to be adjusted for this.

	private function startSession(bool $silence = false) {
		$this->invoke(
			'session_start',
			[
				[
					'cookie_samesite' => 'Lax',
					'read_and_close' => true,
				]
			],
			$silence
		);
	}

What I could not figure out yet is whether this has any negative consequences for our application.

Describe alternatives you've considered

n/a

Additional context

https://www.php.net/manual/en/function.session-start.php, especially the section about this option as well as the comments below that show a few pitfals.

@ChristophWurst ChristophWurst added enhancement help wanted 0. Needs triage Pending check for reproducibility or if it fits our roadmap performance 🚀 labels Oct 21, 2021
@solracsf solracsf changed the title Evaluate PHP's session read_and_close option Evaluate PHP's session read_and_close option Oct 22, 2021
@juliusknorr
Copy link
Member

Read and close might be too limiting by default since we currently always write at least the last activity, but I came up with a slightly different approach to limit the session lock time to only the necessary time slots where the session is actually updated. https://github.com/nextcloud/server/pull/32162/files

juliusknorr added a commit that referenced this issue Aug 16, 2022
… read_and_close

Fixed #29356

Signed-off-by: Julius Härtl <jus@bitgrid.net>
juliusknorr added a commit that referenced this issue Aug 16, 2022
… read_and_close

Fixed #29356

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member

Implemented as a option to configure in #32162

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Aug 16, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants