Add some closeCursor functions to file cache to avoid database locks.

[dragotin]

I'd recommend to add the call to closeCursor() on every sql statement object that is not longer used. I took some time to dig through the sources of doctrine and pdo and the sqlite implementation in particular and it turns out that the sqlite3_finalize() function, that frees the database statement, is only called in the destructor (for the C++ implementation). I don't know when that is called by the PHP engine, but obviously that can be done any time later.

As especially SQLite has a very interesting locking method it can easily happen that a writing operation on a table (INSERT, UPDATE) keeps an lock also for reads as long as the writing operation statement was not closed. The subsequent read operations easily can time out or see a lock because of that.

Apart from that, I think it would be good to set a busy handler timeout explicitely:

$PDO->setAttribute( PDO::ATTR_TIMEOUT, 5 /* Timeout in seconds */ );

but I am not sure where to set that.

This submit request is just one step, I think the closeCursor() functions should be added wherever database interaction happens.

Note: This is not a SQLite only problem, it's just pretty visible there.

[PVince81]

Part of #10566

Seems we have a "tabs vs spaces" situation 😉

[PVince81]

To retest:

• moving files between foldes
• renaming files
• listing files in folders (also covers mime type)
• favorites list (searchByTag)
• file scanner with incomplete entry (?)
• various tests with shared folders (hopefully covering getPathById() and others)

[PVince81]

@DeepDiver1975 set to 8.1

CC @icewind1991

[LukasReschke]

🙈

08:40:13 PHP Fatal error:  Call to a member function closeCursor() on integer in /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/cache/cache.php on line 78
08:40:13 PHP Stack trace:
08:40:13 PHP   1. {main}() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/index.php:0
08:40:13 PHP   2. OC::handleRequest() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/index.php:36
08:40:13 PHP   3. OC\Core\Setup\Controller->run() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/base.php:753
08:40:13 PHP   4. OC_Setup::install() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/core/setup/controller.php:60
08:40:13 PHP   5. OC_User::login() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/setup.php:226
08:40:13 PHP   6. OC_Util::setupFS() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/user.php:219
08:40:13 PHP   7. OC\Files\Filesystem::init() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/util.php:124
08:40:13 PHP   8. OC\Files\Filesystem::initMountPoints() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/filesystem.php:314
08:40:13 PHP   9. OC\Files\Filesystem::mountCacheDir() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/filesystem.php:381
08:40:13 PHP  10. OC\Files\View->mkdir() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/filesystem.php:403
08:40:13 PHP  11. OC\Files\View->basicOperation() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/view.php:185
08:40:13 PHP  12. OC\Files\Cache\Updater->update() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/view.php:815
08:40:13 PHP  13. OC\Files\Cache\Scanner->scan() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/cache/updater.php:54
08:40:13 PHP  14. OC\Files\Cache\Scanner->scanFile() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/cache/scanner.php:235
08:40:13 PHP  15. OC\Files\Cache\Scanner->scanFile() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/cache/scanner.php:130
08:40:13 PHP  16. OC\Files\Cache\Scanner->addToCache() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/cache/scanner.php:165
08:40:13 PHP  17. OC\Files\Cache\Cache->put() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/cache/scanner.php:199
08:40:13 PHP  18. OC\Files\Cache\Cache->buildParts() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/cache/cache.php:243
08:40:13 PHP  19. OC\Files\Cache\Cache->getMimetypeId() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/files/cache/cache.php:299

[dragotin]

The problem now is that the cursors, which do data manipulation, are not closed. That should be done in executeAudited() function, with a check if the call to $stmt->execute($parameters); returns a statement or an integer. In the integer case, the $stmt should be closedCursor()-ed. How can one do the check which type it is in PHP?

[LukasReschke]

How can one do the check which type it is in PHP?

Classes / Objects: http://php.net/manual/de/internals2.opcodes.instanceof.php
Internal types: is_int, is_string etc… (http://php.net/manual/de/ref.var.php)

[nickvergessen]

Related ? #13803

[PVince81]

The problem in core is that there are 3-4 different ways of doing queries. And one of them doesn't support closing cursors. I think we wanted to move away from executeAudited and stuff and use the plain IDBConnection object + statements ?

[PVince81]

@nickvergessen partly related. The end goal is the close cursors everywhere as per #10566

[MorrisJobke]

The intendation is always a bit off (seems to be one space).

What is the state of this?

[DeepDiver1975]

8.2

[MorrisJobke]

@dragotin @DeepDiver1975 Revive or close? Problems seems to be fixed now.

[MorrisJobke]

@DeepDiver1975 @cmonteroluque Move to 9.0 because it is not really ready for review.

[ghost]

@dragotin FYI

[MorrisJobke]

@dragotin @PVince81 @DeepDiver1975 Still valid?

[dragotin]

I have no idea if still valid or not but I would assume so. The problem here is not a concrete bug that is fixed or not. Every closeCursor() in this patch releases resources of the underlying database drivers and unlocks tables that are otherwise locked for a parallel request (IIRC). It turned out that closing these cursors helped, but that is hard to proof as it does not play a role under not so loaded conditions as there is less concurrency (down to no concurrency on devs average development system, which is the source of the problem).

@DeepDiver1975, @nickvergessen and me came up with this patch during debugging of db lock race-conditions.

If you do not wanna use it, drop it. However, I'd ask the database specialist to take it more as an suggestion rather than a concrete PR as probably more closeCursor()-calls could/should be added.

[MorrisJobke]

If you do not wanna use it, drop it. However, I'd ask the database specialist to take it more as an suggestion rather than a concrete PR as probably more closeCursor()-calls could/should be added.

Lately we tried to always close cursors for new connections that are opened. I guess we simply need to clean things up here.

[dragotin]

Yes, it might very well be that this is handled differently/elsewhere or so.

[MorrisJobke]

I would like to close this as we don't have specific issues here currently.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.