-
Notifications
You must be signed in to change notification settings - Fork 120
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: Avoid requesting remote endpoints during bootstrap #3749
Conversation
/backport to stable29 |
try { | ||
$capabilitiesService->fetch(); | ||
$discoveryService->fetch(); | ||
} catch (\Exception $e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do anything here with the caught error? Is it OK to catch it and just throw it away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ignored it as I fear spamming the log here for instances that use richdocumentscode, so I'd rather silence them. The cronjob would still trigger logging of potential errors then.
Will probably look into refactoring that part tomorrow to get rid of those calls.
18a300f
to
db534aa
Compare
This will add some abstraction layer for cached request data as used by capabilities and discovery endpoints. By default it will always return the cached data which will never expire (backed by a file in app data in case the memory cache vanishes). Signed-off-by: Julius Härtl <[email protected]>
db534aa
to
ea60d0e
Compare
$isCODEInstalled = $this->appManager->isEnabledForUser($CODEAppID); | ||
$isCODEEnabled = strpos($this->config->getAppValue('richdocuments', 'wopi_url'), 'proxy.php?req=') !== false; | ||
$shouldRecheckCODECapabilities = $isCODEInstalled && $isCODEEnabled && ($this->capabilities === null || count($this->capabilities) === 0); | ||
if ($this->capabilities === null || $shouldRecheckCODECapabilities) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what has actually caused the issue, as the cache was only there for an hour every request after it expired triggered a new request to the capabilities endpoint, which was unreachable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick review, did not test
Cypress failure also on main, likely some upstream change |
/backport to stable29 |
This will add some abstraction layer for cached request data as used by
capabilities and discovery endpoints. By default it will always return
the cached data which will never expire (backed by a file in app data in
case the memory cache vanishes).
In short the logic is:
There is still a use of fetch for richdocumentscode but that needs some more refactoring to work differently. Does not affect instances where richdocumentscode is not enabled.
Also decreasing the timeout to 5 seconds which should be more then enough.
Summary
TODO
Checklist