Catch put errors

[joeybaker]

db.transaction can throw if the db wasn't properly initialize or if there is a WriteError

[joeybaker]

@jensarps any chance you can take a look at this?

[jensarps]

Sorry for the late reply, I was pondering over this for some time. I'm not really happy with catching errors. Errors are errors, and not being able to write is a biggie and justifies throwing here, I think. If this write operation goes wrong, there's no way you can recover.

Other thoughts I had on this:

• The default error handler would just re-throw the error.
• If we start catching errors there, we would have to do the same in all transactions that require write access, for the sake of consistency
• try/catch blocks will prevent VMs from optimizing the code

I'm not sure if the benefit of having custom error handlers is worth having try/catch blocks in the code – or did I miss other benefits?

[joeybaker]

Apologies for not giving more reasoning in my patch message. Honestly, it's been long enough that I don't remember all the reasons for this change, but to your points:

• yes, the default error handler will just throw, but that is user-configurable. It's weird to provide the user a way to handle errors but not put all errors through that interface.
• for that reason, I'd be down to catch all errors
• yes, the try/catch blocks can't be optimized as well, but isn't that just for the content of the try catch block? Since this is a function call, that function is still optimized no? I guess I fall on the side of not avoiding a key language feature just because JS engines aren't perfectly fast on them right now.

The key benefit I can see is to allow the user to handle the error in their own way … maybe by not throwing and handling the error more gracefully.

[jensarps]

Thanks for getting back on this in detail!

Alright, let's go through the points (I'm really sorry, this is going to b a long one, but this about a pretty substantial change…):

Default Error Handler

Yeah, true, I mean, it's just the default handler – as it's user configurable, it's pretty reasonable to use that mechanism for all error handling.

Catching All Errors

Ok, I see. I wonder what other ops we might want to catch? Regarding the transaction() method, the spec lists following situations where an implementation should throw:

• A "versionchange" transaction is still running.

This is the one that happens most often. Should not happen if the user reads the docs, but if they try to write to the store before the open callback fires, bingo.

• The closePending flag is set.

I can't see that this could actually happen. I guess you could technically achieve this, e.g. when doing stuff in the middle of a deleteDatabase() call, but the odds are pretty low.

• Any of the names in the storeNames array do not exist, or the storeNames array is empty.

Can't happen, as the storeNames array is not exposed to the user.

As I see it, the first one is something we'd want to catch, as it might occasionally happen. But, almost every call to the IndexedDB API could potentially throw. Wrapping transaction() seems reasonable to me, but I don't see other API calls that deserve that kind of attention. Or do you see another potential candidate?

Perf considerations

In V8, a function containing a try/catch block is fully taken out of optimization. The perf impact can be reduced by splitting such a function into two, one containing the try/catch, the other containing the actual work. Still, try/catch is slow by itself. But, I don't think it would hurt too much anyways, as I don't see a scenario where storage ops are performance critical.

In the end, I think we might need to determine between Errors (that is, recoverable issues) and Exceptions (non-recoverable issues that will prevent you from going on). However, I can see the point that a developer wants to be able to maintain control even if a non-recoverable issue occurs, and not having code execution stopped somewhere in the middle of a third party library. I mean, I wouldn't want to see code where each call to IDBWrapper is wrapped in it's own try/catch block.

Executive Summary: Ok, let's wrap transaction() calls, but let's not wrap other API calls. Any objections or thoughts on this?

[joeybaker]

Wow – thanks! Everything you say makes sense. 👍

[jensarps]

Uh, ok, then I guess there's a plan here :)

However, I will not merge this, as I plan to implement the try/catch blocks in a separate function. Also, this might take a while, as I will do some refactoring on the transaction- and callback-handling – there's loads of duplication in there.

I'll leave this open until implemented.

[joeybaker]

Thanks!

[timkuijsten]

FWIW I'd like to add to this that get throws if the key is a Uint8Array: "DataError: Data provided to an operation does not meet requirements." (Fx 45.0a2), as mentioned in Level/level-js#48.

[jayfunk]

Is that exception something indexed db should be throwing or does indexed
db fail in a different manner?

On Wed, Dec 30, 2015, 12:00 Tim Kuijsten notifications@github.com wrote:

FWIW I'd like to add to this that get throws if the key is a Uint8Array:
"DataError: Data provided to an operation does not meet requirements." (Fx
45.0a2), as mentioned in Level/level-js#48
Level/level-js#48.

—
Reply to this email directly or view it on GitHub
#57 (comment).

[timkuijsten]

@jayfunk this is an error from the browser as stated in the spec:

If key is not a valid key or a key range, the implementation MUST throw a DOMException of type DataError.

http://www.w3.org/TR/IndexedDB/#widl-IDBObjectStore-get-IDBRequest-any-key

[jensarps]

@timkuijsten Thanks for the pointer! However, IDBWrapper will not mangle with given keys (it also doesn't change array keys for IE impls).