-
-
Notifications
You must be signed in to change notification settings - Fork 641
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
Comments
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. |
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 |
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. |
@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. |
In theory, per spec (matching @jakearchibald's blog post), the steps for firing success/error would be something like:
There are two bits that are not implemented consistently across browsers:
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. |
@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:
...that is only solved in Chrome. |
Update: Edge seems to have fixed this in Microsoft Edge 38.14393.0.0 / Microsoft EdgeHTML 14.14393. |
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.
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...) |
I have a question. Is this |
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. |
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? |
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.
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. |
Has this issue been reported to Mozilla? Is there a bug I can track? |
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
|
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:
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? |
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) |
@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 |
@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
Going by the docs, all async Dexie methods automatically return a I'll move this to a separate issue if this is not the best place to discuss. |
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. |
@dfahlander yes, saw the Promise-related info in the Best Practices page (great docs btw!). In code we're just using 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 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 |
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):
Maybe there's a way to get it working with |
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? |
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. |
UPDATE: // 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 |
@dperetti Thanks for sharing. Was the non-working transpiled for-await-loop still using native await, yield, or regeneratorRuntime? |
Sorry I was updating my comment while you were replying. See my updated version above !! |
So eventually I don't think it's related to a transpiler vs native issue. I have the same issue in both cases. |
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. |
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. |
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! |
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:
|
Thanks! I've updated those two pages. I also think it's time to close this issue. Thanks everyone involved! <3 |
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:
HATESplays ball with native PromiseHATESplays ball with native Promisedislikes native Promise very muchplays almost perfect ball with native PromiseEDIT: 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.
The text was updated successfully, but these errors were encountered: