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

A workaround for issue 613 #1398

Merged
merged 4 commits into from Nov 19, 2021
Merged

A workaround for issue 613 #1398

merged 4 commits into from Nov 19, 2021

Conversation

dfahlander
Copy link
Collaborator

No description provided.

This version of the workaround is less risky.
It only catches InvalidState errors thrown by IDBDatabase.transaction() and then reopens the database (still only reopen if it hadn't been explicitly closed using db.close()). Caller promise will still wait until reopening is done and the transaction could resume.

Also logging to console.debug() when this happens, so that the occurrance of it can be tracked in production apps.
@trackedsupport
Copy link

Is there a way I can start using this today?

@dfahlander
Copy link
Collaborator Author

dfahlander commented Nov 2, 2021

Is there a way I can start using this today?

Clone or fork this repo and checkout branch workaround-issue-613 . Then do npm install and npm run build in your clone. Then depending if you use npm or yarn

yarn:

  1. yarn link ( when you are in your dexie clone)
  2. cd to your app that depends on dexie and do yarn link dexie

npm:

  1. npm link ( when you are in your dexie clone)
  2. cd to your app that depends on dexie and do npm link dexie

Then rebuild / start your app

@dfahlander
Copy link
Collaborator Author

Please post back here or on issue #613 on how the fix behaves in production.

@trackedsupport
Copy link

Thanks I got that to work but I dont see how this will deploy. In fact, I dont even see how its linked to my dexie clone with any files pointing to the local dexie clone.

@dfahlander
Copy link
Collaborator Author

This is a generic npm/yarn procedure applicable for any project. Please refer to the docs of npm link for a detailed explanation. The PR will also be released in a beta once 3.2.0 stable is out.

@mikekreeki
Copy link

Just for the context, I found this in localforage codebase, seem like the same workaround at the first glance https://github.com/localForage/localForage/blob/master/src/drivers/indexeddb.js#L393-L395

@dfahlander dfahlander merged commit 3e1944c into master Nov 19, 2021
@dfahlander
Copy link
Collaborator Author

A new release dexie@3.2.1-beta.1 was published just now with this PR included. Please try it and report back whether it's a valid workaround for #613 or not.

@trackedsupport
Copy link

trackedsupport commented Nov 19, 2021

Testing now! I just deployed it and I will report back. Im using rollbar and see all the issues from before so hopefully I no longer see them after this deploy.

@trackedsupport
Copy link

I have not seen this issue since I deployed the fix/workaround. Thank you so much!

@trackedsupport
Copy link

i did get this one time. Still much better than what it was! My application does stay on one page for a long time.

Screen Shot 2021-11-24 at 9 01 54 PM

@trackedsupport
Copy link

So I pulled in a local version of Dexie to play around with the PR1398_maxLoop. I set it to 3000 and I am still getting the error. Now they are all from iOS Safari or Chrome so that tells me it could be a bug with iOS. Unfortunately, I don't know when it will be fixed and I don't know what I can do about it except tell the user to stop their recording and refresh the page which is a poor experience.

@trackedsupport
Copy link

One thing I've noticed is that I've seen the error occur on line 1269 which does not have the reopen code.
Screen Shot 2022-03-07 at 3 59 34 PM

@dfahlander
Copy link
Collaborator Author

I've reviewed but as I can see it, that line can only be hit after failed opening of the database. As soon as the workaround is executed, it calls db.open() which synchronically sets db._state.openComplete to false. I can see no race condition because whatever operation in Dexie will from that point start waiting for the open operation to complete. The only case I can see is that for some reason the database has failed to reopen - which would be another situation than what we've worked around in this one - in contrast to the case when exception is thrown when creating a transaction, we don't know whether that situation would possible to work around by retrying or not. It would be interesting to know what the value of db._state.dbOpenError.name is when this line is hit! Notice that this line can also be hit in case a subscriber to db.on('ready') have failed so we cannot just retry every time it happens, but in case we can distinguish the reason by inspecting what error did occur it might be worth it to do a retry for db.open() at this point somehow (in your clone/fork to see whether it would improve the statistics)

@dfahlander
Copy link
Collaborator Author

dfahlander commented Mar 7, 2022

@trackedsupport Did I understand you correctly that the error only occurs on iOS now? As iOS Chrome is just a rebranded Safari this would mean that the chrome-specific bug would be worked around here but it could be a Safari issue we're hitting instead. Is it possible to get the user agent or safari version of the failing clients?

@trackedsupport
Copy link

Hey, So it happens 99% of the time with iOS Safari and Chrome. I am able to recreate with my iPad.

db._state.dbOpenError.name = "UnknownError"

@trackedsupport
Copy link

So I've been running a bunch of tests today with my iPad that kept reproducing the error. I added a db.open() right before line 1269 in the image above but that did not help.

I decided to upgrade my iPad to 15.4 from 15.3 since the issue didn't occur on my iPhone anymore. Sure enough, that worked. So I'm thinking they fixed it in the latest iOS release!

@dfahlander
Copy link
Collaborator Author

👍 Thanks for sharing! That's a good sign we're doing what's possible here for Chrome and that the Safari-specific issue might be solved in Apple webkit.

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

Successfully merging this pull request may close these issues.

None yet

3 participants