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

Chrome - Random Closing Database issue (hard/impossible to reproduce) #613

Closed
jokamax opened this issue Nov 13, 2017 · 32 comments
Closed

Chrome - Random Closing Database issue (hard/impossible to reproduce) #613

jokamax opened this issue Nov 13, 2017 · 32 comments

Comments

@jokamax
Copy link

jokamax commented Nov 13, 2017

Hi,

Some users on Chrome (version > 51 maybe before) encouters in rare case : Database closing (invalidStateError).

The issue (chrome issue) seem to be known : Connection closed on Chrome (IndexedDB) in long sessions

Is there a clean way on dexie to catch that and re-open (without losing data) ?

Or is it possible to make an evolution to automaticaly retry connexion on not wanted closing (option in open init, and the retry is invisible for the code/user during actions on the database) ?

Thanks by advance,

Jonathan

@StabbarN
Copy link

StabbarN commented Jul 26, 2018

This is happening for us too. It seems like some data in indexeddb is lost.

Failed to execute 'transaction' on 'IDBDatabase': The database connection is closing.

@dfahlander
Copy link
Collaborator

dfahlander commented Jul 26, 2018

It's possible to listen to errors globally and take actions. Try listening globally on the 'unhandledrejection' event. Log the error name to console. When you have the error name, you may add code that recovers from it by reopening db. If you find a solution for this chrome issue this way, please share it!

@dfahlander
Copy link
Collaborator

dfahlander commented Jul 26, 2018

Need to say as well, that you will also need to log the event when catching it explicitly, not just rely on the unhandledrejection event, as it will not fire for catched promises.

@trackedsupport
Copy link

trackedsupport commented May 14, 2021

This is happening for us too. It seems like some data in indexeddb is lost.

Failed to execute 'transaction' on 'IDBDatabase': The database connection is closing.

I'm getting this error a lot more now that I switch from saving blobs to saving ArrayBuffers. Im not sure how to fix though. I never close the DB and there is no new version of the DB.

@mikekreeki
Copy link

mikekreeki commented May 15, 2021

For anyone else bumping into this, I took a workaround implemented here https://github.com/yjs/y-indexeddb/blob/ef0a100a40e0102fa4f1ec32130fd75348d4efd2/src/y-indexeddb.js
and seems to work fairly well.

let DB;

const initDB = () => {
  const db = new Dexie(DATABASE_NAME);

  db.version(1).stores(mapping);

  return db;
};

const getDB = () => {
  if (!DB) {
    DB = initDB();
  }

  const idb = DB.backendDB();

  if (idb) {
    try {
      // Check if if connection to idb did not close in the meantime
      idb.transaction('tasks').abort();
    } catch (e) {
      DB.close();
      DB = initDB();
    }
  }

  return DB;
}

// Use it
getDB().table('tasks').add(record)

@dfahlander
Copy link
Collaborator

dfahlander commented May 15, 2021

@mikekreeki thanks for sharing your workaround. It would be marvellous if we could treat this exception within dexie when it happens and reopen the db or possibly fire a custom event to optionally override when it happens. The key would probably be to add code in the generic error handler of transaction in dexie source, that checks for InvalidStateError - and when that happen, try your thing (see if creating a new transaction still works, if not, trigger a reopen or a custom event to define what to do).

I wouldn't think this should be needed but obviously this happens at least on some browsers, so if a built-in workaround could make the indexeddb experience more robust, that would be a great thing.

Just sharing ideas - please remind me of this or if anyone feels inspiration to create a simple PR based on these ideas - must not be complete or pass all tests but the PR could be a starting point or act as a reminder.

@AdventureBeard
Copy link

AdventureBeard commented Sep 28, 2021

Heyo! I'm looking to sponsor work on this particular enhancement (auto-reopen); the unreliability of IndexedDB is wrecking us in prod.

@johannesjo
Copy link

johannesjo commented Sep 28, 2021

@AdventureBeard this might unfortunately be more of a chromium issue, less one of dexie itself. Reading through this might also be helpful to you:
johannesjo/super-productivity#398

@dfahlander
Copy link
Collaborator

dfahlander commented Sep 29, 2021

Heyo! I'm looking to sponsor work on this particular enhancement (auto-reopen); the unreliability of IndexedDB is wrecking us in prod.

The most important thing here is to try a workaround that does not harm existing users performance or stability. If providing a fix based on this idea, I believe we would need to release a new alpha/beta after 3.2.0 stable is out that could be tested by you who experience this in production. Would like to have it running in many production apps before releasing it into an official dexie version. I'll see what I can do.

@dfahlander
Copy link
Collaborator

dfahlander commented Sep 29, 2021

I made a PR #1398. If it pass tests on all browsers, it would need to be tested in production - something I would need your help with. See README.md#contributing on how to symlink your fork of dexie. You'd need to checkout the branch 'workaround-issue-613' before running npm run build.

@AdventureBeard
Copy link

AdventureBeard commented Sep 30, 2021

Wow thanks! Yes, happy to try this out in prod. We've got a big change going out next week; once that's stable we can give it a shot, perhaps week after next if everything goes smoothly.

Any chance of an infinite loop here?

@dfahlander
Copy link
Collaborator

dfahlander commented Oct 4, 2021

Any chance of an infinite loop here?

Firefox private mode throws InvalidStateError on open. However, this happens as a result of the call to open, before we come to the point of creating a transaction, so it wont happen there. I can't imagine any other situation when it would be an infinite loop unless the underlying database it badly corrupt. Could be protected against using a state somewhere, such as in DbReadyState - set on transaction fail, and reset it after successfully creating a transaction (so it can reopen next time it fails).

I think the risk of infinite loop is very theoretical - and it wouldn't be one that consumes all CPU as it would be async requests whithin the iteration. So I hope you could test it as it is before we add that protection.

@mikekreeki
Copy link

mikekreeki commented Oct 16, 2021

Just an update on how my workaround posted in #613 (comment) is performing. Although the users do not end in a state where all subsequent db call fail and restarting the app is required, some db calls still fail and data is probably lost. It seems to happen just a couple of times a day and does not affect any db calls made later. Would #1398 fix such cases as well? Here is what I see in Sentry:

Screenshot 2021-10-16 at 9 41 59

@dfahlander
Copy link
Collaborator

dfahlander commented Oct 17, 2021

Would #1398 fix such cases as well? Here is what I see in Sentry:

Yes, if it works as expected, it should reopen and reexecute a failing transaction transaction so that the using code wouldn't notify any error.

@AdventureBeard
Copy link

AdventureBeard commented Oct 21, 2021

I've deployed this hotfix to production. I'll compile a before/after error report once it has some time to bake. Thanks!

@dfahlander
Copy link
Collaborator

dfahlander commented Oct 26, 2021

I added protection for infinite loop. As soon as 3.2.0 stable is out I will release a 3.2.1-rc with this fix so it's easier to test.

@mikekreeki
Copy link

mikekreeki commented Oct 26, 2021

I will deploy the rc to production as well once it is out. Thanks!

@jokamax
Copy link
Author

jokamax commented Nov 14, 2021

Thank you to all the contributors.
I opened this bugs but I dit not have time to test so : REALLY THANK YOU ALL

@dfahlander
Copy link
Collaborator

dfahlander commented Nov 19, 2021

Now the workaround can be tried using

npm i dexie@3.2.1-beta.1

Please use it for some time and report how it goes.

@dfahlander
Copy link
Collaborator

dfahlander commented Nov 25, 2021

@mikekreeki, @AdventureBeard - do you have some feedback on the workaround (if you've deployed either PR #1398 or dexie@3.2.1-beta.1)?

@mikekreeki
Copy link

mikekreeki commented Nov 25, 2021

@dfahlander I'm sorry, the requirements changed on my end in the meantime and I migrating away from Dexie for unrelated reasons so I won't be able to test it out.

@jokamax
Copy link
Author

jokamax commented Dec 1, 2021

@dfahlander

  1. I will test it locally today and tomorrow
  2. if Ok I will deploy it in production on sunday

@jokamax
Copy link
Author

jokamax commented Dec 5, 2021

  1. Local testing OK
  2. 3.2.1-beta deployed in production.

I'll let it one week and I'll make a return here

@AdventureBeard
Copy link

AdventureBeard commented Dec 7, 2021

@dfahlander , sorry I disappeared! Got distracted with a product release. While I haven't compiled a proper quantitative analysis, I can say anecdotally, we've received fewer reports of the kinds of issues this error caused. A brief glance at our sentry logs seems to indicate these errors are still occurring, albeit at a significantly smaller rate. Sorry this is not as helpful as it could be; currently swamped with product support and travel.

@dfahlander
Copy link
Collaborator

dfahlander commented Dec 7, 2021

Thanks @AdventureBeard! If you would give your analysis an approximate number of the decreased rate, which would be the closest if choosing between

  • 2/3
  • 1/2
  • 1/4 or
  • 1/10

?

@trackedsupport
Copy link

trackedsupport commented Dec 16, 2021

It's definitely helped my application. Any way we can make the max retries a variable that we can set or override? Since my application is open for possibly hours on one page, I would like to reconnect more often than what might be the usual use case.

@dfahlander
Copy link
Collaborator

dfahlander commented Dec 16, 2021

It's definitely helped my application. Any way we can make the max retries a variable that we can set or override? Since my application is open for possibly hours on one page, I would like to reconnect more often than what might be the usual use case.

I think your feedback indicates we should increase the default max retries. The end goal for me would be having the best default number so that other devs wouldn't need to think about it. Your situation with having the app running for hours is not unusual. The only purpose of PR1398_maxLoop is to avoid eternal loop. As soon as a successful transaction takes place, the counter is reset so there's no limit in how many times it may happen.

I suppose it would be safe to increase the default value a bit (20?, in case it helps - clients that still experience the failure despite the workaround may potentially have corrupt DB that wouldn't heal even if trying more times). Can also make it configurable so that you all can help me calibrate a better default.

A PR for this would be nice:

  • Add option to Dexie constructor for setting the value.
  • Give it a better name
  • Increase default to at least 10
  • Use it instead of 3 wherever PR1398_maxLoop is reset.

@trackedsupport
Copy link

trackedsupport commented Dec 18, 2021

I tried to take a look and clone the git to make those changes but I think this is beyond my expertise. I wonder if it was changed to 20 if this would ever be an issue. The number is just to prevent an infinite loop right?

@dfahlander
Copy link
Collaborator

dfahlander commented Dec 18, 2021

@trackedsupport No probs! I'll do it when there's time unless someone else feels inspiration to speed it up.

@trackedsupport
Copy link

trackedsupport commented Dec 18, 2021

@dfahlander Thanks for all the help! If you wouldn't mind since this seems like a simple thing, if you could do one of those changes like change the 3 to a 10, and show me how you do that, run the dist, create the pr etc - that would be a huge help and then I can do the rest. When I opened the code I saw that vairable set all over the place and I believe its because the dist folder is throwing me off but I dont know how to recreate that. This process is just new to me. I had to download the zip because I couldnt figure out how to fork the release/branch/tag.

@dfahlander
Copy link
Collaborator

dfahlander commented Dec 18, 2021

You need to fork dexie repo. Then on your local hard drive where you have the repo copy, do npm install, edit source code under src. Build using npm run build. Edit code in src folder only. Then run npm test before pushing the changes. The PR is created when you have changed the source and pushed the changes to your fork on github. You can create the PR in Github's web gui. To find the places in src, use a code editor like vscode and search all files for PR1398_maxLoop.

You can apply your version and use it in your app also before you have a new official version. There's a section "contributing" in our README that gives some details on how to do npm link etc.

@dfahlander
Copy link
Collaborator

dfahlander commented Feb 16, 2022

Workaround released in dexie 3.2.1

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

7 participants