-
-
Notifications
You must be signed in to change notification settings - Fork 817
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
Support calling close() on cached.Database() #1407
Open
rhansen
wants to merge
1
commit into
TryGhost:master
Choose a base branch
from
rhansen:cached-close
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhansen
added a commit
to ether/ueberDB
that referenced
this pull request
Dec 7, 2020
The `sqlite3.cached.Database()` function returns a Database object that cannot be safely closed. See: TryGhost/node-sqlite3#1407
rhansen
force-pushed
the
cached-close
branch
2 times, most recently
from
February 16, 2021 06:05
aaf95e2
to
bf0d988
Compare
rhansen
force-pushed
the
cached-close
branch
2 times, most recently
from
August 30, 2021 07:38
81d4683
to
92d7288
Compare
Introduce a refcount for cached Database entries so that calling close() on the Database returned from cached.Database() does not invalidate existing or future uses of the same cached Database. This is a backwards-incompatible change because it changes the number of times close() must be called before the underlying Database is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Caching Mechanism: The implementation uses a caching mechanism for SQLite database connections, which can reduce the overhead of repeatedly opening and closing database files.
- Reference Counting: Reference counting with
refCount
helps efficiently manage resources by ensuring that the database connection is only closed when it's no longer referenced. This is useful for preventing resource leaks and optimizing memory use. - Callback Management: The conditional callback check ensures that the provided callback is executed appropriately, either on
open
or usingnextTick
for asynchronous consistency.
Suggestions for Improvement:
- Error Handling: It would be beneficial to add error handling in places where the callback is invoked to handle unexpected cases gracefully. This would be especially useful in
close
whensuper.close
is called. - Code Readability: To improve readability, consider renaming variables like
a
andb
to more descriptive names. For instance,a
could beflags
, andb
could becallback
, making it easier to understand the code's purpose. - Class Extraction: The
Database
class extension within theif (!cacheEntry)
block could be extracted outside of the conditional structure, enhancing readability and potentially making it easier to test. - Singleton Pattern Compliance: Since this code uses a singleton-like pattern for database instances, a further improvement would be to abstract the logic into a
getOrCreateDatabaseInstance
method. This would encapsulate the instance creation and retrieval logic more explicitly.
Minor Tweaks:
- Commenting: A few more inline comments explaining the purpose of each conditional block would improve code maintainability.
- Edge Case Testing: Make sure to test scenarios where
refCount
fluctuates significantly or when the cache entry is deleted during high-concurrency database usage.
Overall, this is a well-thought-out caching and resource management approach for database connections. A few adjustments would enhance the maintainability and robustness of the implementation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a refcount for cached
Database
entries so that callingclose()
on theDatabase
returned fromcached.Database()
does not invalidate existing or future uses of the same cachedDatabase
.This is a backwards-incompatible change because it changes the number of times
close()
must be called before the underlyingDatabase
is closed.