Skip to content

Add cleanup timer for SimpleCache fallback #52

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.

Sign up for GitHub

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

Open
numbpill3d wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add cleanup timer for SimpleCache fallback #52

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions server/utils/performance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,38 @@ try {
console.warn('NodeCache module not found, using fallback implementation');
// Simple fallback implementation
NodeCache = class SimpleCache {
constructor() {
constructor(options = {}) {
this.cache = new Map();
this.stdTTL = options.stdTTL || 0;
this.checkperiod = options.checkperiod || 0;

if (this.checkperiod > 0) {
this._cleanupInterval = setInterval(() => this.cleanup(), this.checkperiod * 1000);
if (this._cleanupInterval.unref) {
this._cleanupInterval.unref();
}

const clear = () => clearInterval(this._cleanupInterval);
process.once('exit', clear);
process.once('SIGINT', clear);
process.once('SIGTERM', clear);
}
}

cleanup() {
const now = Date.now();
for (const [key, item] of this.cache.entries()) {
if (item.expires && now > item.expires) {
this.cache.delete(key);
}
}
Comment on lines +41 to +47
Copy link
Copy Markdown
Contributor

@codereviewbot-ai codereviewbot-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup method in the SimpleCache class iterates over all cache entries to remove expired ones. This approach can lead to performance issues, especially with a large number of cache entries, as it requires a full scan of the cache. Consider optimizing this by maintaining a priority queue or another data structure that can more efficiently track and remove expired entries. This would reduce the time complexity from O(n) to a more manageable level, improving the performance of the cache cleanup operation.

}

set(key, value, ttl) {
const actualTtl = typeof ttl === 'number' ? ttl : this.stdTTL;
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Normalize TTL to a non-negative number

Validate or clamp TTL to ensure it is non-negative before using it.

this.cache.set(key, {
value,
expires: ttl ? Date.now() + (ttl * 1000) : null
expires: actualTtl ? Date.now() + (actualTtl * 1000) : null
});
return true;
Comment on lines 50 to 56
Copy link
Copy Markdown
Contributor

@codereviewbot-ai codereviewbot-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set method in the SimpleCache class always returns true, which does not provide any feedback about the success of the cache insertion operation. This could mask errors or issues during the cache update process. It would be beneficial to implement error handling or at least return a boolean indicating the success of the operation. Additionally, consider throwing exceptions or logging errors when the cache operation fails, to aid in debugging and maintaining the cache logic.

}
Expand Down