Skip to content

Stop cron after update#5172

Closed
nickvergessen wants to merge 2 commits intomasterfrom
stop-cron-after-update
Closed

Stop cron after update#5172
nickvergessen wants to merge 2 commits intomasterfrom
stop-cron-after-update

Conversation

@nickvergessen
Copy link
Member

Ref #4978

Need a way to refresh the OC\Config::$cache array or to ignore it.

The best option I see atm is to pipe a method purgeConfigCache through AllConfig > SystemConfig > Config
The problem is currently we don't re-read the config file, so all this checking is useless...

@nickvergessen nickvergessen added the 2. developing Work in progress label May 30, 2017
@nickvergessen nickvergessen added this to the Nextcloud 13 milestone May 30, 2017
@mention-bot
Copy link

@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @butonic, @icewind1991 and @jakobsack to be potential reviewers.

@blizzz
Copy link
Member

blizzz commented May 30, 2017

The best option I see atm is to pipe a method purgeConfigCache through AllConfig > SystemConfig > Config

Agreed, the alternatives that come to my mind do not look better at all.

@nickvergessen
Copy link
Member Author

A second idea I just had was to automatically invalidate the cache every x seconds, so when a config is get/set later than x seconds, the data is refetched before.

The main problem I start seeing now is, that when two processes run for quite some time, and both write a config value, the later one will overwrite the config value of the first write with the old value it read... time to get rid of the stupid config file :/

@patschi
Copy link
Member

patschi commented May 31, 2017

What about checking for any file modification of the config file itself? Like filemtime or any simple hash function like md5?

@nickvergessen
Copy link
Member Author

Yeah however, then we should do it on every write and save the md5 in memcache or something

@blizzz
Copy link
Member

blizzz commented Jun 13, 2017

A second idea I just had was to automatically invalidate the cache every x seconds, so when a config is get/set later than x seconds, the data is refetched before.

The main problem I start seeing now is, that when two processes run for quite some time, and both write a config value, the later one will overwrite the config value of the first write with the old value it read... time to get rid of the stupid config file :/

Paradise for race conditions. Even without the latter scenario.

@MorrisJobke
Copy link
Member

What is the status of this?

if (time() > $endTime) {
if (time() > $endTime ||
$config->getSystemValue('maintenance', false) ||
$currentVersion !== $config->getSystemValue('version', '')
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure, that this will not work, because the config is already loaded and will still hold the old version.

@MorrisJobke MorrisJobke modified the milestones: Nextcloud 13, Nextcloud 14 Dec 8, 2017
Signed-off-by: Joas Schilling <coding@schilljs.com>
…tive

Signed-off-by: Joas Schilling <coding@schilljs.com>
@MorrisJobke MorrisJobke force-pushed the stop-cron-after-update branch from 7cd7d0c to 9b860d6 Compare May 23, 2018 15:32
@codecov
Copy link

codecov bot commented May 23, 2018

Codecov Report

Merging #5172 into master will increase coverage by 2.43%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #5172      +/-   ##
============================================
+ Coverage     51.71%   54.14%   +2.43%     
+ Complexity    25672    22286    -3386     
============================================
  Files          1634     1379     -255     
  Lines         95813    85346   -10467     
  Branches       1382     1321      -61     
============================================
- Hits          49550    46212    -3338     
+ Misses        46263    39134    -7129
Impacted Files Coverage Δ Complexity Δ
cron.php 0% <0%> (ø) 0 <0> (ø) ⬇️
...are/Security/Exceptions/AppNotEnabledException.php 0% <0%> (-100%) 1% <0%> (ø)
...ware/Security/Exceptions/NotConfirmedException.php 0% <0%> (-100%) 1% <0%> (ø)
apps/workflowengine/lib/Check/RequestUserAgent.php 0% <0%> (-96.43%) 10% <0%> (-4%)
lib/private/Preview/GeneratorHelper.php 0% <0%> (-80%) 5% <0%> (ø)
apps/files_sharing/lib/Helper.php 20.35% <0%> (-58.96%) 38% <0%> (+27%)
apps/files/appinfo/app.php 43.47% <0%> (-56.53%) 0% <0%> (ø)
apps/comments/lib/Activity/Listener.php 24% <0%> (-56.4%) 10% <0%> (ø)
core/Command/Maintenance/Mimetype/UpdateJS.php 0% <0%> (-52.64%) 13% <0%> (ø)
lib/private/Template/JSResourceLocator.php 0% <0%> (-50.91%) 22% <0%> (-1%)
... and 1307 more

@MorrisJobke
Copy link
Member

We splitted this one up into #9561 and #9562 because the approach here is not going to work.

@MorrisJobke MorrisJobke deleted the stop-cron-after-update branch May 23, 2018 15:37
@MorrisJobke MorrisJobke removed this from the Nextcloud 14 milestone May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants