Skip to content
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

feat: support schedule only executes once in random one cluster when it's deployed in k8s clusters mode. #61

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
+192 −6
Open

feat: support schedule only executes once in random one cluster when it's deployed in k8s clusters mode. #61

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6ec7290
✨ feat: avoid run many times for the same job when it's deployed in k…
oneWalker Nov 12, 2024
4fb1881
docs: add readme for the new feature
oneWalker Nov 12, 2024
cf4792b
ci: npm test problem in node 14
oneWalker Nov 12, 2024
190a189
fix: the potential error
oneWalker Nov 12, 2024
d73517d
fix: occupied system
oneWalker Nov 12, 2024
201b1d0
perf: use path.basename to get projectname
oneWalker Nov 12, 2024
aecee0a
feat: -1.abstract base lock for other kinds of locks.2. implement red…
oneWalker Nov 16, 2024
2f46c2c
docs: add usage
oneWalker Nov 16, 2024
176ec2c
perf: enhanced security with AI suggestions
oneWalker Nov 16, 2024
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
18 changes: 18 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,24 @@ config.schedule = {
directory: [
path.join(__dirname, '../app/otherSchedule'),
],
// add it when making sure it only running in one cluster.
cluster: {
enable: false, // default is false, true is for running
lockType: 'redis', //use redis as lock. reserve for other lock type
lockedTtl: 6000, // the automatic expired time for lock, avoid lock leak
// add redis for redis lock
fengmk2 marked this conversation as resolved.
Show resolved Hide resolved
redis: {
client: {
port: 6379, // Redis port
host: '127.0.0.1', // Redis host
password: 'auth',
db: 0,
},
},
},
//the prefix for lockedKey for redis lock
default: 'default', // default schedule name,like project-name.
prefix: 'schedule', // default schedule prefix
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Document Redis key patterns and management

The default and prefix configuration properties need better documentation to help users understand:

  1. The resulting Redis key pattern
  2. Impact on monitoring and debugging
  3. Considerations for multi-application environments

Add detailed documentation:

-  //the prefix for lockedKey for redis lock
-  default: 'default', // default schedule name,like project-name.
-  prefix: 'schedule', // default schedule prefix
+  // Redis key generation configuration
+  // The final Redis key pattern will be: `${prefix}:${default}:${scheduleName}`
+  // Example: "schedule:my-app:clean_cache"
+  default: 'default', // Application identifier, recommended to use your project name
+  prefix: 'schedule', // Global prefix for all schedule locks
+
+  // Example resulting Redis keys:
+  // - schedule:my-app:clean_cache
+  // - schedule:my-app:sync_data
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
//the prefix for lockedKey for redis lock
default: 'default', // default schedule name,like project-name.
prefix: 'schedule', // default schedule prefix
// Redis key generation configuration
// The final Redis key pattern will be: `${prefix}:${default}:${scheduleName}`
// Example: "schedule:my-app:clean_cache"
default: 'default', // Application identifier, recommended to use your project name
prefix: 'schedule', // Global prefix for all schedule locks
// Example resulting Redis keys:
// - schedule:my-app:clean_cache
// - schedule:my-app:sync_data

};
```

Expand Down
9 changes: 9 additions & 0 deletions agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const WorkerStrategy = require('./lib/strategy/worker');
const AllStrategy = require('./lib/strategy/all');
const RedisLock = require('./lib/lock/redis_lock');
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using a lock provider interface for better abstraction.

Instead of directly importing RedisLock, consider creating an abstract lock provider interface to support multiple distributed lock implementations.

Consider this approach:

-const RedisLock = require('./lib/lock/redis_lock');
+const { createLockManager } = require('./lib/lock');

And create a new file lib/lock/index.js:

const RedisLock = require('./redis_lock');

const LOCK_PROVIDERS = {
  redis: RedisLock,
  // Add more providers here
};

exports.createLockManager = (type, agent) => {
  const Provider = LOCK_PROVIDERS[type];
  if (!Provider) {
    throw new Error(`Unsupported lock type: ${type}`);
  }
  return new Provider(agent);
};


module.exports = agent => {
// register built-in strategy
Expand All @@ -11,6 +12,14 @@ module.exports = agent => {
// wait for other plugin to register custom strategy
agent.beforeStart(() => {
agent.schedule.init();
if (agent?.config?.schedule?.cluster?.enable) {
if (
agent.config.schedule.cluster.lockType === 'redis' &&
agent.config.schedule.cluster.redis
) {
agent.lockManager = new RedisLock(agent);
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the gathered information, I can now provide the final response:

Lock manager implementation needs error handling improvements

The lock manager usage in strategy files (worker.js and all.js) shows:

  • Lock acquisition and release are properly paired
  • Locks are only used when cluster is enabled
  • However, there's no error handling around lock operations

Required improvements:

  • Add try-catch blocks around lock operations to handle potential Redis connection failures
  • Ensure locks are released in finally blocks to prevent deadlocks
  • Add error logging for lock-related failures
🔗 Analysis chain

Verify lock manager usage in strategy files.

Ensure that the lock manager is properly used in strategy files with appropriate error handling and lock release.

Let's verify the implementation in strategy files:

Let me gather more information about error handling patterns with a different approach.

Let me try one more search to check for error handling with a corrected pattern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check lock manager usage in strategy files

# Check lock acquisition and release patterns in strategy files
echo "Checking lock patterns in strategy files:"
rg -A 10 "lockManager" "lib/strategy/"

# Check error handling patterns
echo "\nChecking error handling patterns:"
ast-grep --pattern 'try {
  $$$
  this.agent.lockManager
  $$$
} catch'

Length of output: 1427

Script:

#!/bin/bash
# Check the complete implementation context around lock manager usage

# Get broader context of lock usage in worker.js
echo "Lock usage context in worker.js:"
rg -B 5 -A 15 "lockManager" "lib/strategy/worker.js"

# Get broader context of lock usage in all.js
echo "\nLock usage context in all.js:"
rg -B 5 -A 15 "lockManager" "lib/strategy/all.js"

# Look for try-catch blocks in these files
echo "\nTry-catch blocks in strategy files:"
rg -B 2 -A 5 "try\s*{" "lib/strategy/"

Length of output: 1390

Script:

#!/bin/bash
# Check for error handling around the lock manager usage

# Look for try-catch blocks in these files with fixed pattern
echo "Try-catch blocks in strategy files:"
rg -B 2 -A 5 "try {" "lib/strategy/"

# Check if there's any error handling in the parent function
echo "\nFunction context for lock operations:"
ast-grep --pattern 'async function $_($$$) {
  $$$
  lockManager
  $$$
}'

Length of output: 356

}
}
});

// dispatch job finish event to strategy
Expand Down
15 changes: 15 additions & 0 deletions config/config.default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,21 @@ module.exports = () => {
config.schedule = {
// custom additional directory, full path
directory: [],
cluster: {
enable: false,
lockType: 'redis',
lockedTtl: 60000,
redis: {
client: {
port: 6379, // Redis port
host: '127.0.0.1', // Redis host
password: 'auth',
db: 0,
},
},
},
default: 'default',
prefix: 'schedule',
};

return config;
Expand Down
38 changes: 38 additions & 0 deletions lib/lock/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

module.exports = class LockManager {
constructor(agent) {
this.agent = agent;
this.client = null;
this.options = null;
this.prefixKey = `${this.agent?.config?.schedule?.default === 'default' ? this.agent.name : this.agent?.config?.schedule?.default}-${this.agent?.config?.schedule?.prefix}`;
}
Comment on lines +4 to +9
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve constructor's type safety and validation

The constructor needs several improvements:

  1. The prefixKey construction is prone to null reference errors
  2. Missing parameter validation
  3. Lacks comprehensive type definitions

Consider this improved implementation:

+/**
+ * @typedef {Object} Agent
+ * @property {Object} config
+ * @property {Object} config.schedule
+ * @property {string} config.schedule.default
+ * @property {string} config.schedule.prefix
+ * @property {string} name
+ */

+/**
+ * Base class for lock management
+ */
 module.exports = class LockManager {
+  /**
+   * @param {Agent} agent - The agent instance
+   * @throws {Error} If agent or required configuration is missing
+   */
   constructor(agent) {
+    if (!agent) {
+      throw new Error('Agent is required');
+    }
     this.agent = agent;
     this.client = null;
     this.options = null;
-    this.prefixKey = `${this.agent?.config?.schedule?.default === 'default' ? this.agent.name : this.agent?.config?.schedule?.default}-${this.agent?.config?.schedule?.prefix}`;
+    const scheduleConfig = agent.config?.schedule;
+    if (!scheduleConfig?.prefix) {
+      throw new Error('Schedule prefix configuration is required');
+    }
+    const defaultValue = scheduleConfig.default || 'default';
+    const prefix = defaultValue === 'default' ? agent.name : defaultValue;
+    this.prefixKey = `${prefix}-${scheduleConfig.prefix}`;
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
constructor(agent) {
this.agent = agent;
this.client = null;
this.options = null;
this.prefixKey = `${this.agent?.config?.schedule?.default === 'default' ? this.agent.name : this.agent?.config?.schedule?.default}-${this.agent?.config?.schedule?.prefix}`;
}
/**
* @typedef {Object} Agent
* @property {Object} config
* @property {Object} config.schedule
* @property {string} config.schedule.default
* @property {string} config.schedule.prefix
* @property {string} name
*/
/**
* Base class for lock management
*/
module.exports = class LockManager {
/**
* @param {Agent} agent - The agent instance
* @throws {Error} If agent or required configuration is missing
*/
constructor(agent) {
if (!agent) {
throw new Error('Agent is required');
}
this.agent = agent;
this.client = null;
this.options = null;
const scheduleConfig = agent.config?.schedule;
if (!scheduleConfig?.prefix) {
throw new Error('Schedule prefix configuration is required');
}
const defaultValue = scheduleConfig.default || 'default';
const prefix = defaultValue === 'default' ? agent.name : defaultValue;
this.prefixKey = `${prefix}-${scheduleConfig.prefix}`;
}


/**
* Require a lock from lock manager
*
* @param {string} _lockedKey - The key to lock
*/
async acquire() {
// Implementation here
}

/**
* Release a lock from lock manager
*
* @param {string} _lockedKey - The key to unlock
*/
async release() {
// Implementation here
}

/**
* Try to acquire without waiting
*
* @param {string} _lockedKey - The key to try to lock
*/
async tryAcquire() {
// Use _lockedKey in the implementation
// Implementation here
}
Comment on lines +11 to +37
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance abstract methods documentation and contract

The abstract methods need improvements:

  1. Methods should be truly abstract
  2. JSDoc is incomplete
  3. Parameters are defined but unused

Consider this improved implementation:

   /**
    * Require a lock from lock manager
    *
-   * @param {string} _lockedKey - The key to lock
+   * @param {string} lockedKey - The key to lock
+   * @returns {Promise<boolean>} True if lock was acquired, false otherwise
+   * @throws {Error} If lock acquisition fails
+   * @abstract
    */
-  async acquire() {
-    // Implementation here
-  }
+  async acquire(lockedKey) {
+    throw new Error('Method not implemented');
+  }

   /**
    * Release a lock from lock manager
    *
-   * @param {string} _lockedKey - The key to unlock
+   * @param {string} lockedKey - The key to unlock
+   * @returns {Promise<void>}
+   * @throws {Error} If lock release fails
+   * @abstract
    */
-  async release() {
-    // Implementation here
-  }
+  async release(lockedKey) {
+    throw new Error('Method not implemented');
+  }

   /**
    * Try to acquire without waiting
    *
-   * @param {string} _lockedKey - The key to try to lock
+   * @param {string} lockedKey - The key to try to lock
+   * @returns {Promise<boolean>} True if lock was acquired, false if already locked
+   * @throws {Error} If lock attempt fails
+   * @abstract
    */
-  async tryAcquire() {
-    // Use _lockedKey in the implementation
-    // Implementation here
-  }
+  async tryAcquire(lockedKey) {
+    throw new Error('Method not implemented');
+  }

Additionally, consider adding implementation guidance in the class JSDoc:

/**
 * Base class for lock management implementations.
 * 
 * Implementations must:
 * 1. Handle distributed locking mechanism
 * 2. Ensure atomic lock operations
 * 3. Handle lock expiration
 * 4. Implement proper error handling
 * 
 * @abstract
 */

};
70 changes: 70 additions & 0 deletions lib/lock/redis_lock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
'use strict';
const LockManager = require('./base');
const Redis = require('ioredis');

module.exports = class RedisLock extends LockManager {
constructor(agent) {
super(agent);
this.client = new Redis(this.options);
}

/**
* Acquire a lock with waiting time
* @param {string} lockedKey - The key to be locked
* @param {number} expiredTime - The duration in milliseconds for which the lock should be expired automatically
*/
async acquire(
lockedKey,
expiredTime = this.agent.config.schedule.cluster.lockedTtl
) {
// Try again during 5s when it's locked
const start = Date.now();
while (Date.now() - start < expiredTime) {
if (await this.tryAcquire(lockedKey, expiredTime)) {
return true;
}
// Set random sleep time to avoid lock conflicts, random between 0.1s and 1s
const randomSleepTime = Math.random() * 900 + 100;
await new Promise(resolve => setTimeout(resolve, randomSleepTime));
}
return false;
}

/**
* Release a lock from lock manager
* @param {string} lockedKey - The key to be unlocked */
async release(lockedKey) {
try {
lockedKey = `${this.prefixKey}-${lockedKey}`;
await this.client.del(lockedKey);
} catch (err) {
this.logger.error(
`[egg-schedule] ${this.key} release lock error: ${err.message}`
);
}
}
Comment on lines +37 to +45
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure locks are released only by the owner

Currently, the release method deletes the lock key without checking if the releasing instance owns the lock. In distributed systems, it's possible that the lock has expired or was acquired by another process. Consider implementing a mechanism to ensure that only the owner who acquired the lock can release it.

One approach is to store a unique identifier (e.g., a UUID) when setting the lock and check this identifier when releasing the lock.

Example:

// In tryAcquire
const lockValue = `${this.key}-${Date.now()}`;
const result = await this.client.set(lockedKey, lockValue, 'PX', expiredTime, 'NX');

// In release
async release(lockedKey) {
  try {
    lockedKey = `${this.prefixKey}-${lockedKey}`;
    const lockValue = await this.client.get(lockedKey);
    if (lockValue === this.lockValue) {
      await this.client.del(lockedKey);
    }
  } catch (err) {
    this.logger.error(
      `[egg-schedule] ${this.key} release lock error: ${err.message}`
    );
  }
}

This ensures that a lock is only released by the process that acquired it.


/**
* Try to acquire immediately without waiting
* @param {string} lockedKey
* @param {number} expiredTime
*/
async tryAcquire(
lockedKey,
expiredTime = this.agent.config.schedule.cluster.lockedTtl
) {
try {
lockedKey = `${this.prefixKey}-${lockedKey}`;
if (await this.client.get(lockedKey)) {
return false;
}
await this.client.set(lockedKey, true, 'PX', expiredTime);
return true;
} catch (err) {
oneWalker marked this conversation as resolved.
Show resolved Hide resolved
this.logger.error(
`[egg-schedule] ${this.key} try acquire lock error: ${err.message}`
);
return false;
}
}
};
20 changes: 18 additions & 2 deletions lib/strategy/all.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,23 @@
const Strategy = require('./timer');

module.exports = class AllStrategy extends Strategy {
handler() {
this.sendAll();
async handler() {
let canBeLocked = true;
const curConfig = this.agent?.config?.schedule;
let lockedKey = '';
if (curConfig?.cluster?.enable) {
lockedKey = this.key.replace(this.agent.baseDir, '');
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use path manipulation for reliable lock key generation.

The current lock key generation using string replace is fragile and may not work correctly across platforms or when the base directory path appears multiple times in the key.

Consider using path manipulation:

-      lockedKey = this.key.replace(this.agent.baseDir, '');
+      const path = require('path');
+      lockedKey = path.relative(this.agent.baseDir, this.key);

Committable suggestion skipped: line range outside the PR's diff.

if (!(await this.agent.lockManager.tryAcquire(lockedKey))) {
canBeLocked = false;
}
Comment on lines +13 to +15
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling and timeout for lock acquisition.

The lock acquisition needs proper error handling and a timeout to prevent indefinite waiting.

Consider adding try-catch and timeout:

-      if (!(await this.agent.lockManager.tryAcquire(lockedKey))) {
-        canBeLocked = false;
-      }
+      try {
+        const timeout = 5000; // 5 seconds timeout
+        if (!(await this.agent.lockManager.tryAcquire(lockedKey, timeout))) {
+          canBeLocked = false;
+        }
+      } catch (error) {
+        this.agent.logger.error('[egg-schedule] Failed to acquire lock:', error);
+        canBeLocked = false;
+      }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!(await this.agent.lockManager.tryAcquire(lockedKey))) {
canBeLocked = false;
}
try {
const timeout = 5000; // 5 seconds timeout
if (!(await this.agent.lockManager.tryAcquire(lockedKey, timeout))) {
canBeLocked = false;
}
} catch (error) {
this.agent.logger.error('[egg-schedule] Failed to acquire lock:', error);
canBeLocked = false;
}

}

if (canBeLocked) {
this.sendAll();
}
Comment on lines +18 to +20
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling and make execution asynchronous.

The execution block needs proper error handling and should handle potential async operations in sendAll().

Consider adding try-catch and making it async:

     if (canBeLocked) {
-      this.sendAll();
+      try {
+        await this.sendAll();
+      } catch (error) {
+        this.agent.logger.error('[egg-schedule] Failed to execute scheduled task:', error);
+        throw error; // Re-throw to ensure lock is released in the finally block
+      }
     }

Committable suggestion skipped: line range outside the PR's diff.


if (curConfig?.cluster?.enable) {
await this.agent.lockManager.release(lockedKey);
}
Comment on lines +22 to +24
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure lock release with proper error handling.

The lock release should be in a finally block and include error handling to ensure proper cleanup.

Restructure the code:

-    if (curConfig?.cluster?.enable) {
-      await this.agent.lockManager.release(lockedKey);
-    }
+    try {
+      if (canBeLocked) {
+        await this.sendAll();
+      }
+    } finally {
+      if (curConfig?.cluster?.enable) {
+        try {
+          await this.agent.lockManager.release(lockedKey);
+        } catch (error) {
+          this.agent.logger.error('[egg-schedule] Failed to release lock:', error);
+        }
+      }
+    }

Committable suggestion skipped: line range outside the PR's diff.

}
};
22 changes: 19 additions & 3 deletions lib/strategy/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,24 @@

const Strategy = require('./timer');

module.exports = class WorkerStrategy extends Strategy {
handler() {
this.sendOne();
module.exports = class AllStrategy extends Strategy {
async handler() {
let canBeLocked = true;
const curConfig = this.agent?.config?.schedule;
let lockedKey = '';
if (curConfig?.cluster?.enable) {
lockedKey = this.key.replace(this.agent.baseDir, '');
if (!(await this.agent.lockManager.tryAcquire(lockedKey))) {
canBeLocked = false;
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve lock key construction and add error handling

The current implementation has two potential issues:

  1. The lock key construction assumes baseDir is always a prefix of this.key
  2. Lock acquisition errors are silently ignored

Consider applying this diff:

-      lockedKey = this.key.replace(this.agent.baseDir, '');
-      if (!(await this.agent.lockManager.tryAcquire(lockedKey))) {
+      try {
+        // Ensure consistent lock key format across different OS
+        lockedKey = path.relative(this.agent.baseDir, this.key);
+        if (!(await this.agent.lockManager.tryAcquire(lockedKey))) {
+          canBeLocked = false;
+        }
+      } catch (error) {
+        this.agent.logger.error('[egg-schedule] Failed to acquire lock:', error);
+        canBeLocked = false;
+      }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
lockedKey = this.key.replace(this.agent.baseDir, '');
if (!(await this.agent.lockManager.tryAcquire(lockedKey))) {
canBeLocked = false;
}
try {
// Ensure consistent lock key format across different OS
lockedKey = path.relative(this.agent.baseDir, this.key);
if (!(await this.agent.lockManager.tryAcquire(lockedKey))) {
canBeLocked = false;
}
} catch (error) {
this.agent.logger.error('[egg-schedule] Failed to acquire lock:', error);
canBeLocked = false;
}

}

if (canBeLocked) {
this.sendOne();
}
Comment on lines +18 to +20
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling and ensure proper async execution

The task execution block lacks error handling and might not properly handle async operations.

Apply this diff:

     if (canBeLocked) {
-      this.sendOne();
+      try {
+        await this.sendOne();
+      } catch (error) {
+        this.agent.logger.error('[egg-schedule] Task execution failed:', error);
+        throw error; // Re-throw to trigger lock release in the finally block
+      }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (canBeLocked) {
this.sendOne();
}
if (canBeLocked) {
try {
await this.sendOne();
} catch (error) {
this.agent.logger.error('[egg-schedule] Task execution failed:', error);
throw error; // Re-throw to trigger lock release in the finally block
}
}


if (curConfig?.cluster?.enable) {
await this.agent.lockManager.release(lockedKey);
}
Comment on lines +22 to +24
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure lock release with proper error handling

The lock release should be:

  1. Guaranteed to execute using try-finally
  2. Include error handling
  3. Only attempt release if we acquired the lock

Apply this diff to restructure the entire handler method:

   async handler() {
     let canBeLocked = true;
     const curConfig = this.agent?.config?.schedule;
     let lockedKey = '';
+    let lockAcquired = false;
     
     if (curConfig?.cluster?.enable) {
       try {
         lockedKey = path.relative(this.agent.baseDir, this.key);
         if (await this.agent.lockManager.tryAcquire(lockedKey)) {
+          lockAcquired = true;
+        } else {
           canBeLocked = false;
         }
+      } catch (error) {
+        this.agent.logger.error('[egg-schedule] Failed to acquire lock:', error);
+        canBeLocked = false;
+      }
     }

-    if (canBeLocked) {
-      this.sendOne();
-    }
-
-    if (curConfig?.cluster?.enable) {
-      await this.agent.lockManager.release(lockedKey);
+    try {
+      if (canBeLocked) {
+        await this.sendOne();
+      }
+    } finally {
+      if (curConfig?.cluster?.enable && lockAcquired) {
+        try {
+          await this.agent.lockManager.release(lockedKey);
+        } catch (error) {
+          this.agent.logger.error('[egg-schedule] Failed to release lock:', error);
+        }
+      }
     }
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (curConfig?.cluster?.enable) {
await this.agent.lockManager.release(lockedKey);
}
async handler() {
let canBeLocked = true;
const curConfig = this.agent?.config?.schedule;
let lockedKey = '';
let lockAcquired = false;
if (curConfig?.cluster?.enable) {
try {
lockedKey = path.relative(this.agent.baseDir, this.key);
if (await this.agent.lockManager.tryAcquire(lockedKey)) {
lockAcquired = true;
} else {
canBeLocked = false;
}
} catch (error) {
this.agent.logger.error('[egg-schedule] Failed to acquire lock:', error);
canBeLocked = false;
}
}
try {
if (canBeLocked) {
await this.sendOne();
}
} finally {
if (curConfig?.cluster?.enable && lockAcquired) {
try {
await this.agent.lockManager.release(lockedKey);
} catch (error) {
this.agent.logger.error('[egg-schedule] Failed to release lock:', error);
}
}
}
}

}
};
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"dependencies": {
"cron-parser": "^2.16.3",
"humanize-ms": "^1.2.1",
"ioredis": "^5.4.1",
"is-type-of": "^1.2.1",
"safe-timers": "^1.1.0",
"utility": "^1.16.3"
Expand All @@ -36,7 +37,8 @@
"egg-mock": "^5.3.0",
"egg-tracer": "^1.1.0",
"eslint": "^8.29.0",
"eslint-config-egg": "^12.1.0"
"eslint-config-egg": "^12.1.0",
"mocha": "^10.8.2"
Copy link

@coderabbitai coderabbitai bot Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect mocha version number

The specified mocha version ^10.8.2 is invalid as the latest stable version of mocha is 10.2.0. This could be causing the npm test failures mentioned in the PR description.

Apply this diff to fix the version:

-    "mocha": "^10.8.2"
+    "mocha": "^10.2.0"

Additionally, since the project already uses egg-bin for testing, please verify if adding mocha directly is necessary.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"mocha": "^10.8.2"
"mocha": "^10.2.0"

Copy link
Author

Choose a reason for hiding this comment

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

It won't pass the ci in node 14 env without mocha module.

Copy link

Choose a reason for hiding this comment

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

@oneWalker, thank you for the clarification. I've noted that the CI in Node 14 environments requires the mocha module to pass.

✏️ Learnings added 
Learnt from: oneWalker
PR: eggjs/egg-schedule#61
File: package.json:41-41
Timestamp: 2024-11-12T19:35:13.420Z
Learning: In the `egg-schedule` project, adding `mocha` as a devDependency is necessary for the CI to pass in Node.js 14 environments, even when `egg-bin` is used for testing.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

},
"engines": {
"node": ">=14.17.0"
Expand Down