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

Lock SCSS so we only run 1 job at a time #15794

Merged
merged 1 commit into from
Jul 12, 2019
Merged

Lock SCSS so we only run 1 job at a time #15794

merged 1 commit into from
Jul 12, 2019

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented May 29, 2019

This is bit hacky but a start to lock the SCSS compiler properly

Right now it will just fail if the file is locked. This is suboptimal. But for the POC this is fine IMO.

Todo:

  • Busy wait for 10 seconds max until it is done
  • Try to cleanup initialization

@ChristophWurst @MorrisJobke as discussed

@rullzer rullzer added bug 2. developing Work in progress labels May 29, 2019
@rullzer rullzer added this to the Nextcloud 17 milestone May 29, 2019
@skjnldsv skjnldsv added the high label Jun 11, 2019
@skjnldsv skjnldsv self-assigned this Jun 11, 2019
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 13, 2019
@skjnldsv skjnldsv requested review from juliusknorr, icewind1991, ChristophWurst, georgehrke and blizzz and removed request for juliusknorr June 13, 2019 18:10
@ChristophWurst
Copy link
Member

Has anybody been able to reproduce this locally yet?

@skjnldsv
Copy link
Member

@ChristophWurst I tried running multiple same instance after a css cache clear and I got no issue whatsoever :)
I could check with a debug log if this is really triggered properly though.

@ChristophWurst
Copy link
Member

I could check with a debug log if this is really triggered properly though.

That would be awesome!

@skjnldsv skjnldsv requested a review from nickvergessen June 20, 2019 15:20
@skjnldsv
Copy link
Member

I fixed a few stuff.
In the end we should also probably lock the css cache deletion.
If an admin change a variable value and two people refreshes while the cache is being deleted, the cache will be deleted once again.

Also, why is deleting the cache taking so long? @rullzer @nickvergessen @juliushaertl ?

@skjnldsv
Copy link
Member

@rullzer for you :)

@MorrisJobke
Copy link
Member

130 There was 1 failure:
131	
132	1) Test\Template\SCSSCacherTest::testResetCache
133	OCP\ICacheFactory::createDistributed('SCSS-cached-'): OCP\ICache was not expected to be called more than once.
134	
135	/drone/src/lib/private/Template/SCSSCacher.php:371
136	/drone/src/tests/lib/Template/SCSSCacherTest.php:541

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

@rullzer rullzer force-pushed the fix/scss_locking branch from 512294a to b34b5e9 Compare July 12, 2019 14:08
This is bit hacky but a start to lock the SCSS compiler properly
Retry during 10s then give up
Properly get error message
Do not clear locks and properly debug scss caching

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the fix/scss_locking branch from b34b5e9 to f8aeef7 Compare July 12, 2019 14:18
@rullzer
Copy link
Member Author

rullzer commented Jul 12, 2019

Ok lets do this!

@rullzer rullzer merged commit 685f00c into master Jul 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/scss_locking branch July 12, 2019 17:48
@MorrisJobke
Copy link
Member

/backport to stable16

@backportbot-nextcloud
Copy link

backport to stable16 in #16541

@llebout
Copy link

llebout commented May 20, 2020

@rullzer Hello, this is causing extreme slowdowns in production because the lock never gets acquired.

Screenshot Capture - 2020-05-20 - 18-09-19

I think it's never a good idea to sleep in a PHP script because users will be waiting, this totals to 60 seconds of wait time on every single page in Nextcloud, my users are going crazy.

I am on 18.0.4

llebout added a commit to llebout/server-1 that referenced this pull request May 20, 2020
llebout added a commit to llebout/server-1 that referenced this pull request May 20, 2020
Fixes nextcloud#15794 (comment)

Signed-off-by: Leo Le Bouter <lle-bout@zaclys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants