-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
[Bug]: Race condition and misleading exception on concurrent preview generation requests for the same file #41225
Comments
Can't we add a |
As far as i understand, there is an exclusive lock on mkdir already?! server/lib/private/Files/View.php Line 244 in b2c0cd9
mkdir on the local storage is called with hooks ['create', 'write'] , which should acquire a shared lock at firstserver/lib/private/Files/View.php Lines 1139 to 1142 in b2c0cd9
and later changes that to an exclusive lock. server/lib/private/Files/View.php Lines 1148 to 1150 in b2c0cd9
This however doesn't prevent that both requests attempt to create the same folder. For a lock to do so, I think it must be set in Preview/Generator.php::getPreviewFolder and this would collide with the lock later on. Or was your intention to lock the file a preview was requested for? Wouldn't it be quicker to consider in mkdir that the folder might already exist for some reason and return the node to continue with the task that needed that folder? |
Am I understanding this correctly that this error can appear when the folder already exists? Or maybe only if its creation is attempted shortly after its created? An instance I'm administering is getting the same errors, but without deck being installed. In my case the errors appear when imaginary is getting to generate thumbnails, mainly when users are scrolling newly added pictures in the memories app. Example error: NotPermittedException Could not create folder "/appdata_ocfipsbj1v0e/preview/5/8/9/f/7/6/3/7208"{
"reqId": "qziGBcbIZyDSvjrtowZu",
"level": 3,
"time": "2024-05-06T21:19:53+00:00",
"remoteAddr": "193.138.218.201",
"user": "nMax",
"app": "index",
"method": "GET",
"url": "/apps/photos/api/v1/preview/7208?etag=98554f85f4bdf99d151b69b277f80d50&x=512&y=512",
"message": "Could not create folder \"/appdata_ocfipsbj1v0e/preview/5/8/9/f/7/6/3/7208\"",
"userAgent": "Mozilla/5.0 (X11; Linux x86_64; rv:125.0) Gecko/20100101 Firefox/125.0",
"version": "28.0.5.1",
"exception": {
"Exception": "OCP\\Files\\NotPermittedException",
"Message": "Could not create folder \"/appdata_ocfipsbj1v0e/preview/5/8/9/f/7/6/3/7208\"",
"Code": 0,
"Trace": [
{
"file": "/srv/www/nextcloud/lib/private/Files/AppData/AppData.php",
"line": 147,
"function": "newFolder",
"class": "OC\\Files\\Node\\Folder",
"type": "->"
},
{
"file": "/srv/www/nextcloud/lib/private/Preview/Storage/Root.php",
"line": 74,
"function": "newFolder",
"class": "OC\\Files\\AppData\\AppData",
"type": "->"
},
{
"file": "/srv/www/nextcloud/lib/private/Preview/Generator.php",
"line": 607,
"function": "newFolder",
"class": "OC\\Preview\\Storage\\Root",
"type": "->"
},
{
"file": "/srv/www/nextcloud/lib/private/Preview/Generator.php",
"line": 133,
"function": "getPreviewFolder",
"class": "OC\\Preview\\Generator",
"type": "->"
},
{
"file": "/srv/www/nextcloud/lib/private/Preview/Generator.php",
"line": 110,
"function": "generatePreviews",
"class": "OC\\Preview\\Generator",
"type": "->"
},
{
"file": "/srv/www/nextcloud/lib/private/PreviewManager.php",
"line": 187,
"function": "getPreview",
"class": "OC\\Preview\\Generator",
"type": "->"
},
{
"file": "/srv/www/nextcloud/apps/photos/lib/Controller/PreviewController.php",
"line": 178,
"function": "getPreview",
"class": "OC\\PreviewManager",
"type": "->"
},
{
"file": "/srv/www/nextcloud/apps/photos/lib/Controller/PreviewController.php",
"line": 144,
"function": "fetchPreview",
"class": "OCA\\Photos\\Controller\\PreviewController",
"type": "->"
},
{
"file": "/srv/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php",
"line": 230,
"function": "index",
"class": "OCA\\Photos\\Controller\\PreviewController",
"type": "->"
},
{
"file": "/srv/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php",
"line": 137,
"function": "executeController",
"class": "OC\\AppFramework\\Http\\Dispatcher",
"type": "->"
},
{
"file": "/srv/www/nextcloud/lib/private/AppFramework/App.php",
"line": 184,
"function": "dispatch",
"class": "OC\\AppFramework\\Http\\Dispatcher",
"type": "->"
},
{
"file": "/srv/www/nextcloud/lib/private/Route/Router.php",
"line": 315,
"function": "main",
"class": "OC\\AppFramework\\App",
"type": "::"
},
{
"file": "/srv/www/nextcloud/lib/base.php",
"line": 1069,
"function": "match",
"class": "OC\\Route\\Router",
"type": "->"
},
{
"file": "/srv/www/nextcloud/index.php",
"line": 39,
"function": "handleRequest",
"class": "OC",
"type": "::"
}
],
"File": "/srv/www/nextcloud/lib/private/Files/Node/Folder.php",
"Line": 162,
"message": "Could not create folder \"/appdata_ocfipsbj1v0e/preview/5/8/9/f/7/6/3/7208\"",
"exception": [],
"CustomMessage": "Could not create folder \"/appdata_ocfipsbj1v0e/preview/5/8/9/f/7/6/3/7208\""
},
"id": "66408f0280dc8"
} Nextcloud 28.0.5 |
Bug description
For context:
This issue was discoverd with nextcloud/deck#5035 which introduced a new feature that shows attachments of a card not only in the attachment list in the sidebar, but also as cover image to a card.
Uploading an image therefore triggers two concurrent preview generation request with different sizes, leading to a race condition in the Preview Generator.
The effect is that only one of the requests returns the preview image the other throws the missleading Exception:
Opening this issue is a followup to nextcloud/deck#5035 referenced in nextcloud/deck#5214.
I already did some debugging and the main issue occurs at the preview folder creation:
Where does the race condition occur?
It starts at
server/lib/private/Preview/Generator.php
Lines 594 to 605 in 43270c6
As both requests detect that the folder isn't there yet, both will attempt to create the new folder.
To my unterstanding from there it could go different routes dependent on the storage, for Deck it's the local storage.
Here's my debugging message and trace I got for the request that was slower on the local storage and therefore fails to create the new folder:
(Please disregard line numbers, I inserted debugging output that might have shifted things)
What fails is mkdir on the local storage which could be fixed (the quick and dirty solution) by not failing the mkdir if the folder is already exisit:
server/lib/private/Files/Storage/Local.php
Line 114 in 43270c6
Maybe a better solution would be to extend the error handling along the way at
server/lib/private/Files/Node/Folder.php
Lines 156 to 163 in efe68d0
This brings me to the misleading
NotPermittedException
Why misleading exception?
That the mkdir failed does not necessarily mean that this was because of permissions as my debug output shows.
Maybe a
GenericFileException
would be more accurate instead? Something like:I'm a little unsure about this, but shouldn't "checkPermissions(\OCP\Constants::PERMISSION_CREATE)" have already checked whether we have sufficient permissions to create a subfolder?
And why is NonExistingFolder not checking that it is really non existing in its constructor?
Steps to reproduce
Expected behavior
previews are generated regardless of concurrency
Installation method
Community Docker image
Nextcloud Server version
27
Operating system
Debian/Ubuntu
PHP engine version
PHP 8.1
Web server
Apache (supported)
Database engine version
SQlite
Is this bug present after an update or on a fresh install?
Fresh Nextcloud Server install
Are you using the Nextcloud Server Encryption module?
Encryption is Disabled
What user-backends are you using?
Configuration report
No response
List of activated Apps
No response
Nextcloud Signing status
No response
Nextcloud Logs
No response
Additional info
No response
The text was updated successfully, but these errors were encountered: