-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
(#7954) - Connection is reopened, with retry, if automatically closed (idb adapter) #8455
base: master
Are you sure you want to change the base?
(#7954) - Connection is reopened, with retry, if automatically closed (idb adapter) #8455
Conversation
paolosanchi
commented
Feb 17, 2022
•
edited
edited
- The setup process has been moved into the setup() function, so it can be called also in openTransactionSafely()
- The function openTransactionSafely has been moved from utils.js into index.js so it has access to the new setup() function and the data in the adapter (cachedDBs and idb variables).
- The function openTransactionSafely has been changed to be async (with callback).
- Instead of passing the current idb, it's passed the adapter contextualized openTransactionSafely() function.
… closed by the browser
Thank you for your contribution. Would you be able to add a test for this? |
I'd like to, and I understand it is necessary, but I'm not sure how. I'm not very skilled on testing, and I don't know how to test this case since it's an inner behavior. Have you an advice or another view on the thing @AlbaHerrerias? |
Hi @paolosanchi, thanks for identifying this issue and working on a fix for it. My understanding of this PR is that it aims to handle the fact that It might help if I summarise how adapters get started up first -- apologies if
This task queue isn't strictly a queue in that it doesn't make functions Inside the So this design guarantees a couple of important things. First, the adapter setup The new Next there's the question of what the
A lot of this code uses variables scoped to the Finally there's a control flow issue with the fact that It would improve the design if So just to summarise the changes we'd need for this to be safe:
|
Thank you very much @jcoglan for the detailed explanation, I wasn't aware of the meaning of the
If I defer the execution of the adapter's api methods, I should indented almost all the code of the adapter in something like What if we move this handling to the The process would be this:
Can you see any issue whit this? |
You're right to notice that the public API uses callbacks, and there's a lot of callback use internally owing to this being a long-lived codebase. It's still fine to use promises internally where it makes things easier, and the public API actually supports both callbacks and promises --
All I meant here is that public methods -- those in It might be possible to change things so that the task queue can be put back in a "deferred" state if the adapter needs to reconnect, but I suspect there's a lot of code that assumes that once the task queue is ready, it never changes state again so this might be risky. This has reminded me that the newer I think your proposal for putting all the safety logic in A promise is actually a great way of implementing this because you can make sure there's only one active promise representing the DB connection, and then make all calls wait on it. i.e. you'd define a function for opening the DB, that only creates a new connection if let dbPromise = null
function openDB() {
if (dbPromise !== null) {
return dbPromise
}
dbPromise = new Promise((resolve, reject) => {
let openReq = indexedDB.open(dbName)
// do all the connection setup logic...
openReq.onsuccess = (event) => {
resolve(event.target.result)
}
openReq.onerror = (event) => {
reject(event.target.error)
}
})
return dbPromise
} And then you can use this in function openTransactionSafely(stores, mode) {
return openDB().then((db) => {
try {
return db.transaction(stores, mode)
} catch (err) {
dbPromise = null
return openTransactionSafely(stores, mode)
}
})
} This is a simplification and you'd need to use the "cached DBs" store instead of the single |
I also think that this would not always work, because the api public function can do multiple calls to the adapter api and every adapter function opens a transaction, so the connection can drop in between of adapter multiple calls, and putting the queue back to "deferred" state would not simply work, because the check is done at PouchDb level.
It's actually what I tried to explain, so we agree ;) I'm going to implement a new solution with all of these new information in mind, It remains the fact that I totally don't know how to make the integrations tests for this case. |