536798 - Race between canceling async statement and completion notification

Status: RESOLVED INVALID

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Drew Willcoxon :adw]

In bug 490714 I'm running into a race between canceling an async pending statement and notification that the statement successfully finished.  There's a gap at the end of async execution where canceling doesn't really cancel.

In my case, AsyncExecuteStatements::Run on the background thread is preempted by my Cancel call, but after the mCancelRequested check that Run does.  That check is kind of a point of no return, since it's impossible to cancel the statement after it even though I haven't yet been notified that the statement finished.  I was able to solve my problem by checking mCancelRequested one more time in notifyComplete, but that's not guaranteed to work since my Cancel could get scheduled just after that.

There's a second case I haven't run into, but it looks similar.  notifyComplete dispatches a CompletionNotifier to my thread, but between that point and the point where my HandleCompletion is called, it's not possible to cancel.  I think this could be fixed by keeping a reference to the CompletionNotifier and updating its mReason on cancel.

Now, it's possible that execution really has completed successfully before my Cancel call gets to run.  In the second case this is always true, and it's been generally true in my first case.  But it's weird that after I explicitly cancel, the reason subsequently passed to my HandleCompletion is FINISHED and not CANCELED.  If I need to know whether I really canceled (and I do), I'm forced to keep track myself.

http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageAsyncStatementExecution.cpp

Comment 1

[Drew Willcoxon :adw]

Attachment: patch

How's this?

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> In my case, AsyncExecuteStatements::Run on the background thread is preempted
> by my Cancel call, but after the mCancelRequested check that Run does.  That
> check is kind of a point of no return, since it's impossible to cancel the
> statement after it even though I haven't yet been notified that the statement
> finished.  I was able to solve my problem by checking mCancelRequested one more
> time in notifyComplete, but that's not guaranteed to work since my Cancel could
> get scheduled just after that.
That's not true.  There is a cancellation point after each call to sqlite3_step:
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageAsyncStatementExecution.cpp#271

> There's a second case I haven't run into, but it looks similar.  notifyComplete
> dispatches a CompletionNotifier to my thread, but between that point and the
> point where my HandleCompletion is called, it's not possible to cancel.  I
> think this could be fixed by keeping a reference to the CompletionNotifier and
> updating its mReason on cancel.
So, this is intentional (see below).

> Now, it's possible that execution really has completed successfully before my
> Cancel call gets to run.  In the second case this is always true, and it's been
> generally true in my first case.  But it's weird that after I explicitly
> cancel, the reason subsequently passed to my HandleCompletion is FINISHED and
> not CANCELED.  If I need to know whether I really canceled (and I do), I'm
> forced to keep track myself.
I'm not sure how, in the first case, this could actually happen.  The only situation you should be seeing is the second case.  HandleCompletion is supposed to inform the consumer of what happened.  If the consumer happened to cancel, but after the work has been completed, we need to let them know so they can try to undo that work.  This won't matter to read statements, but we don't know what kind of a statement we are given.

Comment 3

[Drew Willcoxon :adw]

Thanks.  As we chatted about, marking invalid.