Make sure cursors are closed everywhere / Database is locked

[PVince81]

This follows my discovery here: #7539 (comment)

In some cases when prepared statements are used, if the cursors aren't closed explicitly, there is a higher risk to run into "Database is locked" issues on SQLite (and maybe max cursors reached on Oracle). We cannot rely on garbage collection to free these resources.

We should go through the core code and find the prepared statements that aren't closed explicitly.
The most likely to cause trouble is ones that might be called repeatedly or the ones defined as class attributes instead of local variables.

What do you think ?

@karlitschek @bantu @icewind1991 @DeepDiver1975 @th3fallen @butonic

[butonic]

a hint: http://php.net/manual/de/pdo.query.php

If you do not fetch all of the data in a result set before issuing your next call to PDO::query(), your call may fail. Call PDOStatement::closeCursor() to release the database resources associated with the PDOStatement object before issuing your next call to PDO::query().

[butonic]

and from http://www.sqlite.org/opcode.html#OpenRead

There will be a read lock on the database whenever there is an open cursor. If the database was unlocked prior to this instruction then a read lock is acquired as part of this instruction. A read lock allows other processes to read the database but prohibits any other process from modifying the database. The read lock is released when all cursors are closed. If this instruction attempts to get a read lock but fails, the script terminates with an SQLITE_BUSY error code.

So, when a SELECT query is executed, but the result set is still open when a modifying SQL statement is executed on the same table we will get the DATABASE LOCKED error.

[PVince81]

Side-note: I'm now struggling with Oracle problems now after adding closeCursor() instructions in my code. Will report later when I found a solution...

[butonic]

did you see http://www.doctrine-project.org/jira/browse/DC-509? it mentions a fix in derflocki/doctrine1@47b926a

quite a dated bug ... I wonder if the fix still applies

[butonic]

Now this seems interesting end exactly the kind of crappy hacking needed to get oracle working http://php.net/manual/de/function.oci-free-statement.php#81577 Now investigating if doctrine does free the cursor AND the statement.

[PVince81]

Not sure, but on my test machine I had to increase the number of cursors because I was getting "Max cursors reached" when running all unit tests on Oracle.

The issue I'm having now is different, see #7539 (comment)
I hope to be able to fix it without having to remove the explicit "closeCursor()".

[PVince81]

I don't think we want to risk introducing regressions in OC 7 by adding cursor closing everywhere ?
Should be fine for OC 8

[PVince81]

@butonic @DeepDiver1975 do you guys have an idea what happened with the "busyTimeout()" setting that we had in OC 7?

[PVince81]

From the stack trace it also looks like it was executed from inside the change propagator's __destruct() method which is automatically called on garbage collection, not sure from where. Maybe at that location there were still open cursors. CC @icewind1991

[dragotin]

Just removed around 30 files locally from the sync directory, the delete propagation to owncloud server post 8.0 release (SHA ac13cf0) has internal server errors (client 1.8.0 beta1):

{"reqId":"2d126becb98ff1d94be833dddd33c5d4","remoteAddr":"::1","app":"remote","message":"An exception occurred while executing 'DELETE FROM \"oc_filecache\" WHERE \"fileid\" = ?' with params [793]:\n\nSQLSTATE[HY000]: General error: 5 database is locked","level":4,"time":"2015-02-17T15:43:26+00:00"}
{"reqId":"88d99c1c3500dc2b46bb920a80f063fa","remoteAddr":"::1","app":"remote","message":"An exception occurred while executing 'DELETE FROM \"oc_filecache\" WHERE \"fileid\" = ?' with params [815]:\n\nSQLSTATE[HY000]: General error: 5 database is locked","level":4,"time":"2015-02-17T15:43:30+00:00"}
{"reqId":"421e19a742f111026b6efd12b94cf0bc","remoteAddr":"::1","app":"remote","message":"An exception occurred while executing 'DELETE FROM \"oc_filecache\" WHERE \"fileid\" = ?' with params [922]:\n\nSQLSTATE[HY000]: General error: 5 database is locked","level":4,"time":"2015-02-17T15:43:39+00:00"}
{"reqId":"9ce133baaa3e7b273ee6cb82c8d5595a","remoteAddr":"::1","app":"remote","message":"An exception occurred while executing 'DELETE FROM \"oc_filecache\" WHERE \"fileid\" = ?' with params [942]:\n\nSQLSTATE[HY000]: General error: 5 database is locked","level":4,"time":"2015-02-17T15:43:40+00:00"}
{"reqId":"CvVy3ulsL1UkUd0sUi8r","remoteAddr":"::1","app":"remote","message":"An exception occurred while executing 'DELETE FROM \"oc_filecache\" WHERE \"fileid\" = ?' with params [952]:\n\nSQLSTATE[HY000]: General error: 5 database is locked","level":4,"time":"2015-02-17T15:46:20+00:00"}

[PVince81]

This seems to be the code for that query: https://github.com/owncloud/core/blob/master/lib/private/files/cache/cache.php#L370

Did you remove only files but also directories ? There seems to be a code path that would also delete child entries.

[PVince81]

The $this->get() does a SELECT on the same table and doesn't clear the cursor. But I'd expect that upon returning from the call, it should garbage collect the cursors automatically.

I also see that $data is actually the result of fetchRow() here: https://github.com/owncloud/core/blob/master/lib/private/files/cache/cache.php#L370 but I don't think that would cause the cursor to be kept.

That might be another argument to always explicitly close cursors and NEVER rely on the garbage collector.

@dragotin I get you aren't able to reproduce this consistently ? We could try and close the cursor there and see if it still happens.

[dragotin]

I removed only files in this case.

I can not really reproduce it, but it shows for me with good frequency when I remove a not too small amount of files locally (lets say 25).

[PVince81]

@dragotin does the client also delete in parallel by sending multiple HTTP requests ?

[PVince81]

Tagging this as junior job: there are many cases where it is easy to change the code to close the cursor.

PRs can be sent separately instead of a huge PR with everything, to make it easier to review and do regression testing on specific pieces of OC.

Complex cases can be left out to be tackled by non-juniors separately 😄

[PVince81]

Had a chat with @dragotin and he found cases where we cannot rely on the garbage collector.
So the decision stays: we need to close the cursors everywhere.

[DeepDiver1975]

@karlitschek @cmonteroluque

To late in the dev cycle - I suggest to postpone this to 9.0 or even backlog - THX

[karlitschek]

yes. too late for proper testing anyways

[ghost]

ok

[ownclouders]

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

[PVince81]

Haven't heard of such issues any more, leaving closed.

[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.