apache / apache/pouchdb

Inconsistent handling of dependent databases during close and destroy

Open
#8,574 2 comments 3 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
17.6k
Forks
1.5k
PR merge metrics
No merged PRs in 30d

Description

### Issue

I noticed some possible inconsistencies while reading the recently added fixes to close dependent databases when closing leveldb databases.

I am absolutely not an expert on PouchDB and its code base, so I may have misunderstood what was done and what was the correct way to handle these cases. Please correct me if I am wrong.

Here are the previous events to understand my concerns:

1. In January 2021, https://github.com/pouchdb/pouchdb/pull/8256 is merged in order to "close leveldb file handlers" directly in the leveldb core adapter. This has been done to fix the issue reported in https://github.com/pouchdb/pouchdb/issues/7331. Note that [no tests were required](https://github.com/pouchdb/pouchdb/pull/8256#pullrequestreview-564905432) to merge this work.
1. In April 2022, the version [7.3.0](https://github.com/pouchdb/pouchdb/releases/tag/7.3.0) of PouchDB is released including this fix.
1. In May 2022, a bug report is submitted in the form of a pull request containing a bug repro (https://github.com/pouchdb/pouchdb/pull/8513). It shows that the fix merged in step 1. poses problems for codebases where multiple PouchDB databases are used side-by-side. Soon after, a pull request is submitted (https://github.com/pouchdb/pouchdb/pull/8515) to fix that issue.
1. In August 2022, having noticed the problem described in step 3. in my own codebase, I started to follow the issue (see https://github.com/pouchdb/pouchdb/pull/8515#issuecomment-1220886629).
1. In November 2022, I noticed that the proposed fix did not ship in release [7.3.1](https://github.com/pouchdb/pouchdb/releases/tag/7.3.1) of PouchDB and asked for guidance on how to help having a fix released. It was rightly suggested that tests were needed (https://github.com/pouchdb/pouchdb/pull/8515#issuecomment-1328328963).
1. I started working on tests but after looking at the codebase, I got very confused about how the things were handled internally and decided I needed more time to understand if fixes proposed in 1. and 3. were properly designed.
1. In the meantime, the fix proposed in step 3. was cherry-picked in the repro pull request (https://github.com/pouchdb/pouchdb/pull/8513) and it got merged to master.

At this point, I could patiently wait for release `7.3.2` to ship and consider the issue fixed.

But I'd like to share what I've seen during work done at step 6. so that more knowledgable people can eventually rework the codebase if the fixes listed above actually introduced inconstencies.

So, here we go.

I first took the time to review how things were designed in the code base:

* In [pouchdb-core/src/adapter.js](https://github.com/pouchdb/pouchdb/blob/24157fc/packages/node_modules/pouchdb-core/src/adapter.js), **common** methods for [destroy](https://github.com/pouchdb/pouchdb/blob/24157fc/packages/node_modules/pouchdb-core/src/adapter.js#L875) and [close](https://github.com/pouchdb/pouchdb/blob/24157fc/packages/node_modules/pouchdb-core/src/adapter.js#L746) actions are implemented, which delegate some work to the underlying adapter chosen.
* In [pouchdb-adapter-leveldb-core/src/index.js](https://github.com/pouchdb/pouchdb/blob/24157fc/packages/node_modules/pouchdb-adapter-leveldb-core/src/index.js), **adapter-specific** implementations for [destroy](https://github.com/pouchdb/pouchdb/blob/24157fc/packages/node_modules/pouchdb-adapter-leveldb-core/src/index.js#L1483) and [close](https://github.com/pouchdb/pouchdb/blob/24157fc/packages/node_modules/pouchdb-adapter-leveldb-core/src/index.js#L1166) methods are available, and are invoked by the common methods previously linked.

The thing that bothers me is that the **common** implementation for [destroy](https://github.com/pouchdb/pouchdb/blob/24157fc/packages/node_modules/pouchdb-core/src/adapter.js#L875) is responsible for destroying dependent databases, whereas due to fixes introduced in steps 1. and 3. above, the **adapter-specific** implementation of the [close](https://github.com/pouchdb/pouchdb/blob/24157fc/packages/node_modules/pouchdb-adapter-leveldb-core/src/index.js#L1166) method is reponsible to close dependent databases.

This seems inconstent, and this might introduce various bugs (such as `_local/_pouch_dependentDbs` referencing databases non longer existing on disk, cf initial use case described in step 1. above (https://github.com/pouchdb/pouchdb/issues/7331)).

In my opinion, dependent databases should be handled consistently either in the **common** implementation, or in the **adapter-specific** implementation, but not with code spread over both sides...

Some reasons to favor the **common** implementation:

* Genericity of the code: other adapters than leveldb may need the same logic
* Anteriority of the pattern: the handling of dependent databases at this level dates back to 2014 ([35cd3abb](https://github.com/pouchdb/pouchdb/commit/35cd3abb7a4ca7c2f516bb204cbc8d42e6b11315#diff-eebe441416ca4cd5569cb2668b496e61ea94e1f351761a1eb5b16c0c75c1accaR113-R130)), while the "recent" leveldb adapter fixes date back to 2021/2022.

I'll submit a pull request soon to demonstrate what it could look like if we handled dependent databases only in the **common** implementation. **Edit** Here it is: https://github.com/pouchdb/pouchdb/pull/8575

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.