Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Browser's incompatibility between native Promise and indexedDB #317

Closed
dfahlander opened this issue Sep 21, 2016 · 35 comments
Closed

Browser's incompatibility between native Promise and indexedDB #317

dfahlander opened this issue Sep 21, 2016 · 35 comments

Comments

@dfahlander
Copy link
Collaborator

dfahlander commented Sep 21, 2016

Background

UPDATE 2019-12-05: Things have changed since this issue was filed:

1. As of the release of Firefox 60 (May 2018), all browsers has become compliant between IndexedDB and their native Promise.
2. Dexie 2.0 has come out with full support for async/await, both native and transpiled, though native async still suffers on older versions of browsers due to this issue.

Site-note: Dexie still works around this limitation in older browsers unless using native (non-transpiled) async functions, which only works well in modern browsers .

/UPDATE

Async await TC39 proposal was finished as of July 2016 and was hard-bound to native Promise. This will be a show-stopper for all non-native promises including Dexie.Promise from entering the async/await sugar world. Transpilers such as Babel and Typescript has until now left an open door for custom promises by allowing the use of a local var Promise = CustomPromise at current module scope. But we simply cannot rely on this anymore.

Problem

So why can't we just make Dexie use native Promises and get rid of this issue? Here's the MAJOR reason:

IndexedDB Transactions does not work with native Promises

A recent test made on evergreen browsers at September 21, 2016, verifies the following unpleasant information:

Chrome / OperaIndexedDB plays ball with native Promise
FirefoxIndexedDB HATES plays ball with native Promise
EdgeIndexedDB HATES plays ball with native Promise
SafariIndexedDB dislikes native Promise very much plays almost perfect ball with native Promise

EDIT: Edge has fixed this in latest version Microsoft Edge 38.14393.0.0 / Microsoft EdgeHTML 14.14393
EDIT2: Safari 10.1 has fixed this as well. However, keep away from micro tasks after a transaction has been created (No Promise.resolve().then(...) after transaction creation without using it).
EDIT3: Firefox 60 has fixed this also.

Only in Chrome, Opera and latest Edge version, IndexedDB transactions plays well with native Promises. In Safari, it will keep the transaction alive between single Promise.then() calls, but if you wait for several Promise.resolve() after each other, and then continue to try using the transaction, your screwed!

Dexie works around this issue by implementing its own micro-task loop, making its Promise implementation A+ compliant without sacrificing indexedDB compatibility. But now when TC39 has finalized the async / await standard to rely exclusively on native Promise, not only Dexie, but other indexedDB libraries as well, are in a situation where we can't do async/await with indexedDB and target other than Chromium browsers.

Way Forward

If all browsers played well with native Promises, we're actually not that far from switching to generate native Promises in Dexie. We could make it work and still retain transaction state. There is commented-out code in Dexie that would make it possible to use native Promise. The reason it's been commented out is due to the fact presented in this issue - turning it on would lie to our users, saying they could use native promises with Dexie when the fact is it wont work on other platforms than Chromium.

On the other hand, in case all of Edge, Safari and Firefox would FIX THIS within a year or so, it might be useful after a while, when all browsers out there are updated. But is is worth it? Is it really going to happen so soon? Given the time it has taken for browser vendors to support indexedDB so far (several years), It doesn't seem likely that all of them should fix in years to come.

So I believe we will change our docs to discourage people from using async await in favour of using yield. That will save it for plain javascript users but be a pain in the XX for typescript users (See #315). They'll simply need to use the Promise-style within Dexie transactions, or maybe use yield if it could provide any type safety, which I doubt.

EDIT: Dexie has the support now for native promises and async/await ever since version 2.0. And it makes more sense now as all modern browsers have solved the issue!

Conclusion

No library on earth will support async/await within IndexedDB transactions and run that code on Firefox or Safari for the coming two years at least (my simple guess). Making Dexie support native Promises will not change that. Due to that fact, we should discourage the use of async / await within transactions for now. .

EDIT: The above text was written september 2016 but all browsers became compliant in may 2018. Dexie >= 2.0 works with native async functions on all modern browsers (Edge, Chrome, Opera, Safari and Firefox). And ff transpiling the code with Typescript or Babel, it can target also the older versions of browsers including Firefox <60. So I won't be discouraging async functions anymore.

dfahlander added a commit that referenced this issue Sep 21, 2016
dfahlander added a commit that referenced this issue Sep 21, 2016
* Removed samples for ES7 and Typescript
* Update according to issue #317
@dfahlander dfahlander changed the title Support for native async await Incompatibility with native async await Sep 21, 2016
dfahlander added a commit that referenced this issue Sep 22, 2016
Updated samples to reflect #317 + bower.json ignore (#319) + fix of Dexie.Promise.resolve(jqueryPromise) should return a Dexie.Promise.
@dfahlander
Copy link
Collaborator Author

Ok, so I've played around a little and made a branch that supports native promises without having to wrap stuff in Dexie.Promise. This turns out to work with native async await in Edge. It will most likely work on in typescript 2.0 as well. Please checkout my new branch experiment/support-native-promise and test it in angular / zone.js so see if it copes there as well.

However, the incompability between indexedDB and native promises in Edge, Safari and Firefox persist, so using async await will still fail in Edge with TransactionInactiveError, as I already knew. This fix may only be useful for developers targeting Chromium exclusively.

@nolanlawson
Copy link

I am confused. I thought it was by-design that transactions would autocommit if you yielded to a microtask? I'm actually kinda surprised Chrome even has this behavior; it may be a bug.

Take a look at @inexorabletash's https://github.com/inexorabletash/indexeddb-promises repo, where he introduces a waitUntil() method specifically to work around the problem of Promises causing the transaction to auto-commit. Also your input on inexorabletash/indexeddb-promises#12 would be appreciated @dfahlander. :)

@dfahlander
Copy link
Collaborator Author

I don't know if Chromium is violating the standard or not. It may be for a rational reason since IMHO this mutual incompatibility between indexedDB and native Promise is a bug in the overall Web platform.

IndexedDB was designed before the platform had an async primitive. Now when it has, I think it would be very much irrational not updating the indexedDB spec to reflect how async primitives are handled.

@dpogue
Copy link
Contributor

dpogue commented Sep 27, 2016

@nolanlawson https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/ does a good job of explaining where microtasks fit into the microtasks/tasks/events loop.

It sounds like the IDBTransaction remains active until the success event is fired. That is a task, so promise microtasks should run before that, while the transaction is still active.

@inexorabletash
Copy link

inexorabletash commented Sep 27, 2016

In theory, per spec (matching @jakearchibald's blog post), the steps for firing success/error would be something like:

  1. Update the request state
  2. Create the event
  3. Set the transaction to active
  4. For each target in (request, transaction, connection), do:
    1. If event propagation is not stopped, then for each listener on target, do:
      1. Run listener's callback
      2. Run microtasks
  5. Set the transaction to inactive

There are two bits that are not implemented consistently across browsers:

  • Microtasks should run every time control returns from script
  • The transaction is active around the whole thing

I believe some browsers only execute microtasks after each target is processed (i.e. at an imaginary 4.2 step), and some do it at the end of the whole task (i.e. at an imaginary step 6). This leads to the observed behavior differences in IDB, although it's not IDB specific - e.g. with a click event you will also see differences in when microtasks run.

@dfahlander
Copy link
Collaborator Author

dfahlander commented Sep 30, 2016

@inexorabletash It seems that Safari and Edge is actually doing microtasks correctly for promises (ran @jakearchibald's tests on them today and they behave like Chrome now!). Problem seems to be the second one:

The transaction is active around the whole thing

...that is only solved in Chrome.

@dfahlander
Copy link
Collaborator Author

Update: Edge seems to have fixed this in Microsoft Edge 38.14393.0.0 / Microsoft EdgeHTML 14.14393.
@nolanlawson, did you fix it ;) ?

@dfahlander dfahlander changed the title Incompatibility with native async await Browser's incompatibility between native Promise and indexedDB Oct 12, 2016
robertknight added a commit to robertknight/passcards that referenced this issue Oct 17, 2016
In Firefox v52, LocalStore#saveKeys() triggered a
TransactionInactiveError when trying to concurrently save the encryption
key and password hint to the database. The request to save the key
started a new IDB transaction and the request to save the hint attempted
to re-use that transaction.

This worked before when using Q.Promise but broke with the recent switch
to native Promises, possible due to the issues discussed in
dexie/Dexie.js#317

Since transaction re-use is just an optimization, the problem is worked
around for the moment by just avoiding re-use.
@nolanlawson
Copy link

I didn't fix it, but yeah, lots of IDB stuff has been improved recently and continues to get improved (even while we don't have multiEntry and friends yet...)

@sechel
Copy link

sechel commented Nov 22, 2016

I have a question. Is this var Promise = CustomPromise business supposed to solve the issue of mixing Dexie and native promises in a transaction? During debugging I see how the local Promise is replaced in the transaction. But Promise values returned by, e.g., the Webcrypto API are still native Promise implementations and break the transaction. Do I miss something here? Here is my setup:
dexie 2.0.0-beta.4
typescript 2.1.1 with target es6

@dfahlander
Copy link
Collaborator Author

Webcrypto API will break IDB transactions no matter the workarounds done by Dexie to make promises interact. Actually, Dexie transaction zones are maintained also for native promises originating from api:s lite WebCrypto and fetch(). The problem lies in IndexedDB in that it will make the transaction fire off when calling a non-IndexedDB async API.

There is a new proposal going on that would enable IndexedDB transactions to wait for arbritary promises to complete. Until then, we cannot use WebCrypto within transactions.

To store encrypted data, I would suggest either to encrypt it before starting the transaction, or, use a javascript-based (synchronous) crypto API instead of WebCrypto.

@sechel
Copy link

sechel commented Nov 22, 2016

Aha. In my current situation I need to perform WebCrypto operations during the transaction of a database upgrade. As I cannot shift away from WebCrypto I prefer your first suggestion and prepare the data I need for the upgrade in advance. Here is my question: Is there any mechanism that I can use that hooks in right before the upgrade transaction is created?

@dfahlander
Copy link
Collaborator Author

@sechel, let's continue on #374 where I have suggested a way to accomplish what you need to do.

dfahlander added a commit that referenced this issue Apr 28, 2017
Unit tests fails for Firefox 53. The unit tests that fails are those that test native async-await support.

As described in #317, browser vendors (now only Firefox and Safari) still have incompatibility between their native Promise and their native IDBTransaction implementations. Dexie works around this by using its own Promise implementation, but when it comes to native async/await, we are totally in the hands of the native promise compatiblity. Chrome, Opera and Microsoft Edge have solved their previous incompatibility issues and they support both native async/await and Promise/IDB compatibility. Firefox used to lack both, but now they created the async/await support without solving the Idb/Promise incompatibility.

In summary, unit tests now do a feature test whether there is compatibility between IndexedDB transactions and Promise or not. If not compatible, we won't run those unit tests since they would fail badly with TransactionInactiveError.
@dpogue
Copy link
Contributor

dpogue commented Aug 28, 2017

The test in the linked gist appears to pass in Safari 10.1 on desktop (and most likely iOS Safari 10.3). That leaves Firefox as the only browser where IDB and Promises don't play nicely.

@BobVul
Copy link

BobVul commented Oct 20, 2017

Has this issue been reported to Mozilla? Is there a bug I can track?

@dfahlander
Copy link
Collaborator Author

@inexorabletash
Copy link

Thanks for pushing on this.

If you have cycles, can you look at the following tests to make sure we're covering Dexie's expected behavior fully?

The first two are probably the most important relevant. In: https://github.com/w3c/web-platform-tests/tree/master/IndexedDB

  • event-dispatch-active-flag.html
  • transaction-deactivation-timing.html
  • upgrade-transaction-deactivation-timing.html
  • upgrade-transaction-lifecycle-backend-aborted.html
  • upgrade-transaction-lifecycle-committed.html
  • upgrade-transaction-lifecycle-user-aborted.html

@poltak
Copy link

poltak commented Mar 3, 2018

Just became aware of this issue after trying my Dexie code in FF and seeing it fail. Bit confused about the current status of this though. By this:

EDIT: Dexie 2.0 now works with native async functions on all modern browsers (Edge, Chrome, Opera, Safari and Firefox). This means native async await works well on all browsers except for Firefox, due to the incompatibility described here. If transpiling the code with Typescript or Babel, it can target also IE and Firefox

Does this mean there is no way to use IndexedDB transactions - and, in-turn, Dexie transactions - in Firefox right now, or I can simply update my build to transpile my asyncawait code (or don't touch it?) and it will work?

@dpogue
Copy link
Contributor

dpogue commented Mar 3, 2018

Relevant: Mozilla Intent to implement and ship: spec compliance Promise microtask behavior posted 2 days ago. They are aiming to get the spec compliant behaviour into Firefox 60 (which will also be an Extended Support Release)

@dfahlander
Copy link
Collaborator Author

@poltak no, Dexie.Promise fixes it. Only issue is if you use non-transpiled async/await with Firefox, as it forces the use of the builtin promise

@poltak
Copy link

poltak commented Mar 3, 2018

@dfahlander Thanks for the reply. Played around with build and confirmed babel is transpiling asyncawait (tried both into es6 generators and es5), however still getting issues in FF 59. To clarify, in case this is completely unrelated, any transaction performed throws a Dexie.PrematureCommitError - have also looked over transaction code to ensure no external/non-Dexie async calls

no, Dexie.Promise fixes it

Going by the docs, all async Dexie methods automatically return a Dexie.Promise rather than native Promise, so there's nothing I need to do manually here, like window.Promise = Dexie.Promise, right?

I'll move this to a separate issue if this is not the best place to discuss.

@dfahlander
Copy link
Collaborator Author

No, you don't need to use Dexie.Promise explicitly to get it working, but in case you use Promise.all(), race or resolve(), new Promise (), make sure to not import Promise. Promise polyfills are supported but they should set window.Promise before app code starts, not be imported. Otherwise, PrematureCommitError will happen. If you'd have a chance to post a link to the failing repo (or fiddle/plunk), that would be great.

@poltak
Copy link

poltak commented Mar 4, 2018

@dfahlander yes, saw the Promise-related info in the Best Practices page (great docs btw!). In code we're just using (window.)Promise everywhere, but realised babel was overriding that with core-js polyfill. Played around with changing the way babel polyfills, and also manually overriding window.Promise with different promise polyfills in main entrypoint, but still end up with the PrematureCommitError in FF.

It is on the branch associated with this PR. It's actually a web extension, and you need to load as a temporary addon in FF, so maybe not so fun to test. Instructions if you really want to. The Dexie code is all in src/search-index/search-index-new dir, starting from the index module - and there is some gulp spaghetti in gulpfile.babel.js and of course babel settings in .babelrc.

It's probably just something fairly simple I'm not doing right with the build though. Will spend more time playing with it this week, so please don't inconvenience yourself too much

@poltak
Copy link

poltak commented Mar 5, 2018

Managed to get it working. It was indeed just something with the build.

To briefly outline, for anyone else with the same problem and similar build setup (although may not be an issue for much longer in FF):

  • previously using babel-plugin-transform-runtime to automatically handle polyfilling a lot of things
  • there's a polyfill flag you can supply to change the polyfill scope from global (assigning window.Promise I suppose) and aliasing (replacing some code like new Promise() with new _promise() where _promise is assigned to the polyfilled constructor)
  • transactions in FF seem to prematurely commit with both strategies (makes sense with aliasing, as it conflicts with Best Practices page's Promise info)
  • I ended up removing babel-plugin-transform-runtime and changing build to just use babel-preset-env along with useBuiltIns flag + import 'babel-polyfill' in all my script entrypoints (useBuiltins replaces the entire babel-polyfill import with only the ones you need for your target env/s)
  • with babel-preset-env, you just need to ensure you have a target env that will include core-js.es6.promise polyfill (or specify it in the include option to force it)

Maybe there's a way to get it working with babel-runtime that I missed, and my JS build tools knowledge is very limited - so take this info with a grain of salt - but I'm just happy to have found a solution at this point. Thanks for your replies @dfahlander

@WillAvudim
Copy link

Is it true that this issue is long gone and native promises can now be used in Firefox, just like in all other modern browsers? Or all the updates are current?

@dperetti
Copy link

For the records, I'm facing very weird PrematureCommitErrors in Electron 3.04 since I switched to using Babel 7. I'm able to reproduce it in my app but it involves very specific and non intuitive conditions, so I'm not sure I can reproduce it in a minimal example.
However, I traced my code and came to the conclusion it didn't not match the caveats mentioned here.
It rather looked like some sort of leak.
Since I had upgraded to Babel 7 right before and thought it was related, I tried to compile using the @babel/plugin-proposal-async-generator-functions plugin and it worked again!
So my understanding is that Babel 7 uses native Promises with Electron, and that they are not ready for prime time...

@dperetti
Copy link

dperetti commented Nov 15, 2018

UPDATE:
Here is a tricky one !
I get PrematureCommitErrors when using for await on an empty iterable.

   // items = []
   for await (const s of items)) {
       await this.db.items.put(new ItemEntry({/* ... */}))
   }
   // ... more calls 
   await this.db.otherItems.put(new OtherItem({/* ... */))
}
// PrematureCommitError here when the transaction exits

@dfahlander
Copy link
Collaborator Author

@dperetti Thanks for sharing. Was the non-working transpiled for-await-loop still using native await, yield, or regeneratorRuntime?

@dperetti
Copy link

Sorry I was updating my comment while you were replying. See my updated version above !!

@dperetti
Copy link

So eventually I don't think it's related to a transpiler vs native issue. I have the same issue in both cases.

@dperetti
Copy link

I know Promise.all() returns synchronously when it is passed an empty iterable. For await probably has a similar behavior that can be related to this issue.

@dfahlander
Copy link
Collaborator Author

@dperetti - I've breaked out your issue into #774, let's continue there.

@birtles
Copy link

birtles commented Dec 5, 2019

Is it true that this issue is long gone and native promises can now be used in Firefox, just like in all other modern browsers? Or all the updates are current?

Firefox 60 (released 2018-05-09) should have compliant microtask handling for Promises. I notice that https://github.com/jakearchibald/idb doesn't have any specific guidance about handling Firefox so I assume it works. I also ran this gist through mozregression and confirmed that it works as expected as of Firefox 60.

I suggest the Dexie docs need to be updated to remove the guidance about native Promises in Firefox since the latest Firefox ESR is Firefox 68 which contains this fix.

@dfahlander
Copy link
Collaborator Author

dfahlander commented Dec 5, 2019

Thanks @birtles. I thought I had already updated the docs, but I suppose there are pages with old information still. Do you have a link to the page(s) that has inaccurate info? Also, please feel free to click the edit button and correct them directly - it will generate a PR for the dexie.js-web repo.

EDIT Oh, I saw this very same issue had inaccurate info. I just updated the main issue text!

@birtles
Copy link

birtles commented Dec 5, 2019

Do you have a link to the page(s) that has inaccurate info? Also, please feel free to click the edit button and correct them directly - it will generate a PR for the dexie.js-web repo.

Huh, I totally didn't notice the edit button! Unfortunately I'm out of office for the next few days so in case I forget to come back to it, a couple of instances I came across today were:

@dfahlander
Copy link
Collaborator Author

Thanks! I've updated those two pages. I also think it's time to close this issue. Thanks everyone involved! <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants