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.

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

Fix the time calculation problem caused by start_time #844

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

JinRiYao2001
Copy link

Description:

This pull request addresses an issue when setting a specific start_time for scheduled tasks in django-celery-beat. When the start_time is set to a future time, Celery treats this time as part of the event_t tuple, specifically the event_t.time field. As a result, Celery continues to check whether the task has reached its execution condition, which can cause unwanted behavior.

Changes:

  • Modified the is_due method in django-celery-beat/schedulers.py to prevent tasks with a future start_time from being processed before the start_time is reached.

Expected Behavior:

The event.time should be generated based on the start_time, the current time, and the task's schedule, ensuring that tasks with a future start_time do not appear at the top of the heap or get processed until their execution time is reached. This approach allows other tasks, such as interval-based tasks, to execute without being blocked.

issue

@JinRiYao2001
Copy link
Author

Changes:

  • Modified the delay calculation logic in is_due()
  • Now, delay is calculated based on start_time, now, and schedule.remaining_estimate()
  • This ensures a more accurate computation of the next execution time

Why is this change needed?

  • Previously, delay was calculated as start_time - now, which is not always accurate
  • The new logic ensures that the delay aligns with the scheduler's expected execution time

Impact:

  • The unit test test_task_with_start_time may fail due to the updated delay calculation
  • The test still assumes delay = start_time - now, which does not match the new logic
  • It is necessary to update the test to align with the schedule.remaining_estimate() calculation

Suggested Solution:

  • I propose updating test_task_with_start_time to validate the new delay calculation logic

Question

  • Should I submit an updated unit test along with this change?

@Nusnus Nusnus marked this pull request as draft February 9, 2025 16:46
@Nusnus
Copy link
Member

Nusnus commented Feb 9, 2025

Changed to draft until the CI passes

@JinRiYao2001
Copy link
Author

I'll take the time to fix this code

@JinRiYao2001
Copy link
Author

I found that self.schedule.remaining_estimate cannot calculate the next execution time based on start_time, but this part of the code is implemented in Celery. Therefore, I should modify Celery's code and implement a method in Celery's code to calculate the next execution time for each task.

After implementing the corresponding method, I will call it within django-celery-beat.

@JinRiYao2001
Copy link
Author

JinRiYao2001 commented Feb 10, 2025

I created a method for the PeriodicTask model to calculate the accurate start_time, and it calls is_due to calculate the delay when the task is crontab.

This way, I don't need to modify the Celery source code, and I fixed this issue in django-celery-beat. I'm not sure if this kind of modification follows the standard practices?

@JinRiYao2001
Copy link
Author

I ran the unit tests for django-celery-beat, and they pass on my Mac but fail on Windows. I would like to use the official CI to run the tests.

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.85%. Comparing base (c34fdcd) to head (9c68012).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
+ Coverage   87.42%   87.85%   +0.42%     
==========================================
  Files          32       32              
  Lines         954      963       +9     
  Branches       76       77       +1     
==========================================
+ Hits          834      846      +12     
+ Misses        102      100       -2     
+ Partials       18       17       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JinRiYao2001
Copy link
Author

I discovered a bug in my code while trying to write unit tests. I will fix it later and conduct the corresponding tests.

@auvipy auvipy self-requested a review February 11, 2025 09:50
@JinRiYao2001
Copy link
Author

JinRiYao2001 commented Feb 11, 2025

In the process of supplementing the test cases, I found that my code still has some time zone issues, which I will modify tomorrow.

@JinRiYao2001
Copy link
Author

I think I solved the time zone issue.

@JinRiYao2001 JinRiYao2001 marked this pull request as ready for review February 11, 2025 18:06
@Nusnus Nusnus requested a review from cclauss February 11, 2025 18:23
@JinRiYao2001 JinRiYao2001 requested a review from auvipy February 12, 2025 10:59
@JinRiYao2001 JinRiYao2001 requested a review from cclauss February 13, 2025 08:24
@auvipy
Copy link
Member

auvipy commented Mar 11, 2025

I will re review it tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants