Shawn Wilsher :sdwilsh Assignee
	Description • 16 years ago

Right now it's really hard to run a series of statements that need proper ordering with the async API.  This isn't terrible to fix.

The hard part about this is how to handle a transaction.  These operations would likely want to be batched in a transaction.  However, there are a few issues:
1) a transaction could already be open - how do we handle that?
   a) wait until we get one (don't think this is good - see (2))
   b) carry on without it, and hope everything goes OK
2) transactions are not thread independent, so while we may be able to get a transaction, someone else can dump something in us without a problem.

I'm not really sure what a good solution is here, so feedback welcome.

	Shawn Wilsher :sdwilsh Assignee
	Comment 1 • 16 years ago

This wasn't that bad - just had to convert AsyncExecute to handle more than one statement.

	Shawn Wilsher :sdwilsh Assignee
	Comment 2 • 16 years ago

Posted to dev.platform about the proposed interface change:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/690e85855bdbbc55#

	Dave Camp (:dcamp)
	Comment 3 • 16 years ago

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

>diff --git a/storage/src/mozStorageConnection.cpp b/storage/src/mozStorageConnection.cpp

>+
>+nsresult
>+mozStorageConnection::ExecuteAsync(mozIStorageStatement ** aStatements,
... 
>+        if (!stmts.AppendElement(new_stmt)) {
>+            rc = SQLITE_NOMEM;
>+            break;
>+        }
>+
>+        // Reset this statement
>+        (void)aStatements[i]->Reset();
>+    }

As mentioned on irc, in the unlikely event of a failure partway through, you have a partially-reset array.  Reset should probably be an all-or-nothing thing.

	Dave Camp (:dcamp)
	Comment 4 • 16 years ago

and if you reset statements even in the face of failure, that should probably be doc'ed.

	Shawn Wilsher :sdwilsh Assignee
	Comment 5 • 16 years ago

(In reply to comment #4)
> and if you reset statements even in the face of failure, that should probably
> be doc'ed.
I think it's covered by "These statements can be reused immediately, and reset does not need to be called."

	Shawn Wilsher :sdwilsh Assignee
	Comment 6 • 16 years ago

Addresses review comments, and addresses one sr comment made in person.

	Shawn Wilsher :sdwilsh Assignee
	Comment 7 • 16 years ago

http://hg.mozilla.org/mozilla-central/rev/60e6f428bd13

	Eric Shepherd [:sheppy]
	Comment 8 • 16 years ago

This has been documented for a while:

https://developer.mozilla.org/En/MozIStorageStatement

	Shawn Wilsher :sdwilsh Assignee
	Comment 9 • 16 years ago

(In reply to comment #8)
> This has been documented for a while:
> 
> https://developer.mozilla.org/En/MozIStorageStatement
Not there - this bug is about adding the same method name to mozIStorageConnection, which takes an array of mozIStorageStatements.

	Eric Shepherd [:sheppy]
	Comment 10 • 16 years ago

Oops.  OK, sorry about that.

	Eric Shepherd [:sheppy]
	Comment 11 • 16 years ago

But it's already documented there, too.

https://developer.mozilla.org/en/mozIStorageConnection#executeAsync%28%29

:)

	Shawn Wilsher :sdwilsh Assignee
	Comment 12 • 16 years ago

uh, my bad...

	Eric Shepherd [:sheppy]
	Comment 13 • 16 years ago

Not really -- I did look in the wrong place; I just lucked out it had been done already in both places.  Sorry about that confusion.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 14 • 16 years ago

Marking as verified fixed by seeing all tests pass and no backout happened.