-
-
Notifications
You must be signed in to change notification settings - Fork 18
refactor: improve Redis client connection handling and error management to prevent app shutdown #1449
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
Conversation
…nt to prevent app shutdown
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.
Pull request overview
This PR refactors the Redis client connection handling in the DataAccessObjectRedis class to improve error management and prevent application shutdown. The changes focus on adding proper error handlers, implementing a reconnection strategy, and improving TLS configuration.
Key Changes:
- Added try-catch block around client creation with proper cache cleanup on errors
- Implemented reconnection strategy with exponential backoff (max 3 retries)
- Refactored TLS/SSL configuration logic to conditionally apply based on connection settings
- Added error event handler to log errors and clean up cached clients
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LRUStorage.setRedisClientCache(this.connection, client); | ||
| reconnectStrategy: (retries: number) => { | ||
| if (retries > 3) { | ||
| return new Error('Max reconnection attempts reached'); |
Copilot
AI
Nov 24, 2025
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.
The reconnectStrategy should throw an error or return false to stop reconnection attempts, not return an Error instance. According to the Redis client documentation, the function should either:
- Return a number (milliseconds to wait before next retry)
- Return false or throw an Error to stop reconnection
Current code returns an Error object which will be treated as a truthy value and converted to NaN for the delay.
Recommended fix:
reconnectStrategy: (retries: number) => {
if (retries > 3) {
return false; // or throw new Error('Max reconnection attempts reached');
}
return Math.min(retries * 100, 3000);
},| return new Error('Max reconnection attempts reached'); | |
| return false; |
|
|
||
| if (shouldUseTLS) { | ||
| socketConfig.tls = true; | ||
| socketConfig.rejectUnauthorized = this.connection.ssl === false ? false : true; |
Copilot
AI
Nov 24, 2025
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.
The logic for rejectUnauthorized is incorrect. When shouldUseTLS is true, this line will always evaluate to true because it checks this.connection.ssl === false (which is false when TLS is enabled).
The current logic:
- When
this.connection.ssl !== false(TLS enabled):rejectUnauthorized = false ? false : true→true - When
this.connection.ssl === false(TLS disabled): This block won't execute anyway
This should be simplified or corrected. Consider:
socketConfig.rejectUnauthorized = this.connection.ssl !== false;Or if you need explicit control over certificate validation, use a dedicated property rather than overloading the ssl boolean.
| socketConfig.rejectUnauthorized = this.connection.ssl === false ? false : true; | |
| socketConfig.rejectUnauthorized = this.connection.ssl !== false; |
| client.on('error', (err) => { | ||
| console.error('Redis Client Error:', err); | ||
| LRUStorage.delRedisClientCache(this.connection); | ||
| }); |
Copilot
AI
Nov 24, 2025
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.
Using console.error for logging in a library/shared-code module is not ideal. Consider using a proper logging framework or allowing the error to bubble up through the existing error handling mechanism.
Additionally, the error handler removes the client from cache but doesn't handle the case where the client might be in an error state but still cached. This could lead to subsequent calls trying to use a failed client before it gets removed.
Consider either:
- Using a proper logger that's injected or configured for the application
- Emitting events that can be handled by application-level logging
- At minimum, making logging configurable
| if (!client) { | ||
| const shouldUseTLS = this.connection.ssl !== false; | ||
|
|
||
| const socketConfig: any = { |
Copilot
AI
Nov 24, 2025
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.
The type annotation any for socketConfig bypasses type safety. Consider using a proper type from the Redis client library or defining a specific interface for the socket configuration.
For example:
import type { RedisSocketOptions } from 'redis';
const socketConfig: Partial<RedisSocketOptions> = {
// ...
};
No description provided.