Re: [IndexedDB] Should .add/.put/.update throw when called in read-only transaction?

I've thought about this more and have some additional doubts inline.

On Thu, Jul 1, 2010 at 11:55 AM, Jonas Sicking <jonas@sicking.cc> wrote:

> On Wed, Jun 30, 2010 at 6:42 PM, Jeremy Orlow <jorlow@chromium.org> wrote:
> > On Thu, Jul 1, 2010 at 11:17 AM, Jonas Sicking <jonas@sicking.cc> wrote:
> >>
> >> Hi All,
> >>
> >> Currently the IndexedDB specification is silent on what should happen
> >> if IDBObjectStore.add, IDBObjectStore.put, IDBObjectStore.remove,
> >> IDBCursor.update or IDBCursor.remove() is called from a READ_ONLY
> >> transaction. There are two possible ways we can handle this:
> >>
> >> 1. We can throw an exception.
> >> 2. We can return a IDBRequest object and asynchronously fire a 'error'
> >> event on this object.
> >>
> >> The advantage of 1 is that we pretty much know that this was an error
> >> due to a bug in the web page,
>

I don't see why this is compelling.  Many of the other errors that you still
propose we fire via the callback are due to bugs in the page.


> >> and we can always know this
> >> synchronously without having to consult the database.
>

So?  Just because we can doesn't mean we should.


> >> Throwing an
> >> error means that all the existing infrastructure for error handling
> >> with automatically kick in. For example any higher-level try/catch
> >> constructs will have an opportunity to catch the error.
> >> Implementations generally report uncaught exceptions to an error log.
> >> The browser will fire an 'error' event on the window which the page
> >> can use for further logging. Firing an error event on the other hand
> >> does not allow the browser to automatically log the error in a console
> >> as the page hasn't yet gotten a chance to handle it.
>

Sure, but this doesn't help the majority of error conditions in IndexedDB.
 It also ignores the cost of handling errors in 2 different ways.


> >> The advantage of 2 is that this is consistent with other error
> >> conditions, such as writing duplicate keys, disk errors during writing
> >> the database to disk, internal errors in the database, etc.
>

The other problem is that users then need 2 sets of error handling routines
for each call.  Given how difficult it is to get web developers to do any
error checking, requiring 2 types of checks seems like a big downside.


> >> While consistency, and only needing to check for errors one way, is
> >> certainly good arguments, I would argue that people won't need to
> >> check for calling-add-on-read-only-transactions. For properly written
> >> code it's not an error that will occur, and thus there is no need to
> >> check for it. In fact, you probably are generally better off letting
> >> the exception bubble all the way up and get logged or caught by
> >> generic error handlers.
>

These are awfully bold assumptions.  Simply not catching the error is not an
option for many web applications or libraries.  At very least, they'll need
to add finally statements to handle such cases.

>> Additionally, the structured clone algorithm, which defines that an
> >> exception should synchronously be thrown if the object is malformed,
> >> for example if it consists of a cyclic graph. So .add/.put/.update can
> >> already throw under certain circumstances.
> >>
> >> Also compare to if we were using a different API strategy of making
> >> objectStores and cursors returned from READ_ONLY transactions not have
> >> mutating functions. In this case if someone tried to call .put(), that
> >> also would result in a exception from the JS interpreter stating that
> >> you're calling a function that doesn't exist.
> >>
> >> So I would argue that we should throw for at least all transaction
> >> violations. I.e. whenever you try to perform an action not allowed by
> >> the current transaction. This would also cover the case of calling
> >> createObjectStore/removeObjectStore/createIndex/removeIndex during a
> >> non-setVersion-transaction.
> >>
> >>
> >> There is also another case where synchronously know that an error will
> >> be reported. We could throw when IDBCursor.update() is called when the
> >> underlying object store uses in-line keys and the property at the key
> >> path does not match the key in this cursor's position. In this case we
> >> similarly immediately know that there is an error without having to
> >> consult the database. We also generally can be sure that there is a
> >> bug in the web page which would benefit from being reported like other
> >> bugs are.
> >>
> >> And like stated above, IDBCursor.update() can already throw if the
> >> passed in object can't be structurally cloned.
> >>
> >>
> >> Jeremy previously asked if there was a test we could use to
> >> clearly/intuitively break error conditions into two groups. Ones that
> >> cause exceptions to be thrown, and ones that cause error events to be
> >> fired. I would say that errors that do not depend on what data is in
> >> the database, but rather are clearly due to errors at the call site
> >> should throw an exception.
> >
> > This would limit us in the future in terms of schema changes.  The
> current
> > async interface differs starting the transaction until the first call
> that
> > accesses/modifies data (which are all async).  If we ever allow a schema
> > change to happen without disconnecting all clients, it'd be possible that
> > the objectStore could be deleted between when the call is made and when
> the
> > transaction is actually allowed to start.
>
> I'm not quite following here. Even if we in the future allow
> objectStores to be deleted while there are transactions open against
> it, then .add/.put would still know if we're inside a READ_ONLY or
> READ_WRITE transaction, no? And so could still throw an error if we're
> in a READ_ONLY transaction.
>
> By the test defined above, .put would in that situation have to fire
> an error event, rather than throw, if properly called on an READ_WRITE
> transaction, but where the objectStore had been deleted. This because
> we would have to check with the database if the objectStore was
> deleted and thus would fail the "do not depend on what data is in the
> database" check.
>

I don't see how the existence of an objectStore can be considered "data".
 Sure an objectStore _contains_ data and that data would no longer be
accessible, but I don't see how the existence can be considered anything
other than meta data.

> This also will limit what can be done on a background thread.  For
> example,
> > an implementation couldn't do serialization of the object on a background
> > thread (yes, if you did this, you'd need to make sure the main thread
> didn't
> > modify it until it finished serializing).
>
> For what it's worth, HTML5 already defines that the structured clone
> algorithm throws an exception, so that's not really something
> introduced by me in this thread.


Is there any technical reason this has to throw rather than raising the
error through the callback?  I don't see one.  We already made the change to
WebKit so that we can opt out of throwing on a serialization error.


> I also think that the problem you
> describe in parenthesis effectively prevents you from doing background
> serialization, at least without first copying so much data that you
> could also perform the constraints checks at the same time.
>

Some implementations might be able to copy on write efficiently.  You could
also put a lock around the data, lock it before main thread execution begins
again, and have the main thread take the lock before modifying the data.

This was also just one example of processing that could be done in the
background.  And even if none of them are very compelling, keep in mind that
we're backing ourselves into a corner in terms of what we can add in the
future (without breaking this heuristic).


> > Because of these reasons, I'm not too excited about this particular
> > heuristic for when to throw vs fire an error callback.  I've thought
> about
> > it a bit and can't think of anything better though, unfortunately.
> > I think I'm still slightly in favor of routing all errors through onerror
> > callbacks and never throwing from a function that returns an IDBResult,
> but
> > I think there were some good points brought up by Jonas for why throwing
> on
> > some errors would make sense.
>
> I think HTML5 already forces us to make .put/.add/.update throw in
> certain circumstances. And I think the benefits that come with
> exception handling outweigh the theoretical possibility of performing
> the structured clone in a background thread.
>

As I mentioned earlier in the thread, these were examples of small problems
in the current spec that I expect will really come back to bite us in the
future as we try to add more complex features.  And I'm not convinced HTML5
forces us to do anything.

After thinking more about this, I'm not totally against this proposed
change, but I also don't find the arguments in its favor very compelling.

J