451851 - Need a more reliable way to indicate SQLITE_BUSY

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

right now it and SQLITE_LOCKED are mapped to NS_ERROR_FILE_IS_LOCKED, but that isn't a good error message for storage.  I think it's high time we get some more useful status codes, starting with this one.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 345311 [details] [diff] [review]
v1.0

Yeah, the semantics are rather different for the errors, so it's nice to have them mapped to different errors.  Also, nice to not have two include files with the same name anymore.

Since you're introducing a new error code for SQLITE_BUSY, I think it makes sense to modify mozStorageStatement::ExecuteStep to return it when it gets a SQLITE_BUSY.  Right now it explicitly returns NS_ERROR_FAILURE (in the same case as SQLITE_MISUSE...)  r=asuth with that.

(nb: mxr for NS_ERROR_FILE_IS_LOCKED suggests no one currently cares about this error; not that it is likely to have been emitted with ExecuteStep converting it explicitly.)

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Since you're introducing a new error code for SQLITE_BUSY, I think it makes
> sense to modify mozStorageStatement::ExecuteStep to return it when it gets a
> SQLITE_BUSY.  Right now it explicitly returns NS_ERROR_FAILURE (in the same
> case as SQLITE_MISUSE...)  r=asuth with that.
I would do that, except that consumers who are checking for exact error codes will break.  As much as I want to be better about that, I think it might be better to wait for mozilla 2 for that kind of change (and making all the error codes map better in general).

Comment 4

[Shawn Wilsher :sdwilsh]

Er, with that said, are you OK with landing as is?

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

So, I buy your logic, but I'm not sure it's consistent.  The return values of SQLITE_BUSY for mozStorageConnection::executeSimpleSQL and its users beginTransaction, commitTransaction, and rollbackTransaction change with this patch.  Do you have a particular set of existing consumers that you know use ExecuteStep in this fashion but nothing else, or are we dealing with hypothetical cases?

Having said that, I'm fine with landing this; an improvement is an improvement.

Comment 6

[Shawn Wilsher :sdwilsh]

hmm, yeah.  We should just update ExecuteStep.  There is nothing in tree that I'm aware of.

Comment 7

[Shawn Wilsher :sdwilsh]

I removed the |return NS_ERROR_FAILURE;| line when we check for SQLITE_BUSY or SQLITE_MISUSE, so it will fall out and end up calling ConvertResultCode.

Comment 8

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/fd8dd4d3ff48