dmonad / lib0

Monorepo of isomorphic utility functions
MIT License
345 stars 63 forks source link

IDB blocked event forces a page reload #55

Closed raineorshine closed 10 months ago

raineorshine commented 1 year ago

Checklist

Is your feature request related to a problem? Please describe.

When calling idb functions, it is possible that the page is forced to refresh in rtop when onblocked triggers:

https://github.com/dmonad/lib0/blob/8af099f94f78194ead544b1893d1a386e6efedea/indexeddb.js#L19-L28

Perhaps there is something causing the db to be blocked in my specific case that I can investigate. Currently it is preventing me from calling clearData in y-indexeddb.

However, I'm concerned with the low-level behavior here. The risk of a forced page refresh in a production app is pretty bad. I can't think of a case where this would be a positive user experience.

Describe the solution you'd like

The app should be able to handle the blocked event with its own logic, at least by opting in, if not by default. This would allow one to ignore the blocked idb request, retry the request, show a message to the user, etc.

I would recommend rejecting with a custom Error type that can be caught and detected higher up.

Describe alternatives you've considered

location.reload cannot be monkey patched, so there is very little that can be down currently to stop it.

I'm happy to help with a PR if this is a valid request.

dmonad commented 1 year ago

I think you are right that the Promise should be rejected with a custom Error. It makes sense at least from an API perspective. However, let me give this feedback before you open a PR:

Once a request is blocked you probably are in a very bad application state. I don't see how you can resolve a blocked database connection except by reloading the window (or by finding the blocked resource and freeing it).

If your issue arises from calling clearData, you probably still have a database connection open which prevents you from opening another instance.

We could reject the request as if an error occurred. But, again, I have no idea how I would resolve such an issue with this information. I also don't want to force users to try {} catch {} every request. Hence, location.reload sounds like a good fallback in almost all cases. Being able to catch an error is only useful if you know how to get back to a valid state again.

raineorshine commented 1 year ago

A request may only be blocked temporarily, unless I am missing something. For example, some update triggers an idb call, and at the same time the user hits a delete button that calls clearData. location.reload is a poor fallback in this case.

dmonad commented 1 year ago

I'd love to get a reproducible test case.

In my experience, this issue only occurs when you open two separate indexeddb instances to the same database. If this happens, it is usually hard to close one of the database objects. Also see MDN: https://developer.mozilla.org/en-US/docs/Web/API/IDBOpenDBRequest/blocked_event

Maybe this is bad design on my part because clearData can't be called on an existing database object. My intention was that clearData should only be called when the original database object has been deleted.

In any case, I think you are right and the promise should be rejected. Happy to accept a PR.

priyadarshy commented 1 year ago

I'd like to chime in and say that we're experiencing this issue and it's creating a terrible user experience downstream (the whole web app reloads). If we had a way to handle this besides a reload we could provide a more graceful reconnection in our application. I also looked into monkey patching reload, which isn't possible. We are still figuring out how to get a reproducible test case. It seems this happens very, very rarely but when it does it makes our application unusable for users.

We'd very much welcome a PR here as well!

raineorshine commented 1 year ago

Here is a test case that reproduces a blocked event when the databases is deleted:

https://codesandbox.io/s/lib0-idb-blocked-test-00cknl?file=/src/App.tsx

Notably, the database is still successfully deleted if the blocked event is ignored completely, even without retries. Ignoring the blocked event in my app works great with a lot more concurrent actions (a document created and deleted on every key stroke). All of the databases get cleaned up correctly once the requests are allowed to resolve.

I'm sure there are some cases where a blocked event requires special handling. However, given that ignoring the blocked event seems to be correct in both cases I tried, I would suggest emitting the event for consumption, but not throwing an exception.

I'll open a PR with your preferred solution.

dmonad commented 1 year ago

Thank you, this is super valuable to understand what is going on!

There is not much documentation on the blocked event on mdn. But according to w3c the "blocked and upgradeneeded events may be fired at an open request to indicate progress." (source)

Hence, I agree that we can safely ignore the blocked event as we don't know how to handle it anyway. The success or error event will be fired eventually (or never).

Looking forward to your PR!

dmonad commented 10 months ago

Fixed in #57