890203 - Make runInBatchMode to run batch asynchronously

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect)

Description

[Raymond Lee [:raymondlee]]

We need to find a way to make runInBatchMode to run batch asynchronously.

See Bug 885205 comment 20

Comment 1

[Asaf Romano (gone)]

This blocks bug 891303 with no apparent temporary work around.

Comment 2

[Asaf Romano (gone)]

The solution proposed in bug 885205 comment 20 won't help transaction aggregation.

Each transaction wishes to run on its own most of the time, but at times multiple transactions may be aggregated together, in which case it's appropriate to start a batch before the first transaction is about to be applied, and end it whenever the last transaction is fully applied, or if any transaction failed to apply.

Ideally, runBatched could return a promise, but since runInBatchMode is implemented in c++ we cannot do that.

I think we have no choice but reviving the API that preceded runInBatchMode, which is the beginBatch-endBatch couple, alongside the runInBatchMode API. Then, in PlacesUtils we could easily implement a promises-friendly version of runInBatchMode.

This will work fine, and I don't expect it to result in any in-tree bugs, because beginBatch/endBatch will never be used directly except by the PlacesUtils wrapper. However, addons may go ahead and use the internal API, and I'm not sure what we can do about that.

Comment 3

[Asaf Romano (gone)]

One thing I could do to make addon authors at least feel guilty for using begingBatch is to force a *get*Interface call for using this method, so PlacesUtils would do something like

runBatchec: function(aFunction) {
  return Task.spawn(function() {
    let placesUtilsHelpers = this.history.QueryInterface(Ci.nsIInterfaceRequestor)
                                 .getInterface(Ci.nsPIPriavatePlacesUtilsHelpers);
    placesUtilsHelpers.beginBatch();
    try {
      yield aFunction();
    }
    finally {
      placesUtilsHelpers.endBatch();
    }
  };
}

The reason for using getInterface rather than QueryInterface is that getInterface doesn't leave the private methods exposed on PlacesUtils.history.

Comment 4

[Asaf Romano (gone)]

Marco, are you ok with this approach?

Comment 5

[Asaf Romano (gone)]

Alternatively, the PlacesUtils helper could just call |BeginTransaction...Commit| on the places DB connection directly. It won't be a problem for runInBatchMode, because mozStorageTransaction does nothing if there's already a storage transaction in progress.

Comment 6

[Asaf Romano (gone)]

The "only" problem with that approach is onBeginBatch/onEndBatch listeners.

Comment 7

[Asaf Romano (gone)]

To be clear, the listeners issue consists of two separate issues:
1) If runInBatchMode was called while an "async batch" was already in progress,
   onBeginBatch shouldn't be called. That's easy to fix, becuase 
   mozStorageTransaction::HasTransaction can tell you whether or not it generated a "real"
   transaction. Similarly, when runInBatchMode ends, onEndBatch shouldn't be called if an
   async batch is in progress. That's also easy to fix using DBConn::GetTansactionInProgress
2) If an "async batch" has been started by PlacesUtils and generated "a real transaction",
   onBeginUpdateBatch should be called, and when async batch completes, onEndUpdateBatch
   should be called if no transaction is in progress. We'll nned to extend
   nsPIPlacesHistoryListenersNotifier in order to do that.

Comment 8

[Marco Bonardo [:mak]]

as discussed over IRC, I think it's fragile to base on GetTransactionInProgress, due to concurrent usage of sync and async statements we do.
before moving to a complicate management for batch notifications, I think we should try to investigate if it's possible to get rid of batch notifications completely, by making the result smart about changes.
I will try to make an experimental patch doing that and see.

Comment 9

[Marco Bonardo [:mak]]

We should rather do bug 1340498.