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

Correct calculation of application current time with timezone #4173

Merged
merged 2 commits into from Jul 28, 2017

Conversation

Projects
None yet
@georgepsarakis
Member

georgepsarakis commented Jul 28, 2017

Description

Incorrect application current time calculation was introduced with https://github.com/celery/celery/pull/3867/files .

  • Use datetime.astimezone to adjust current time in App.now.

Closes #4160 .

@georgepsarakis

This comment has been minimized.

Member

georgepsarakis commented Jul 28, 2017

@rtpg @thedrow @auvipy can you please review this change? It seems that it has many side-effects. See also celery/kombu#774 and probably #4169 .

@@ -880,8 +880,11 @@ def prepare_config(self, c):
def now(self):
"""Return the current time and date as a datetime."""
from celery.utils.time import to_utc

This comment has been minimized.

@thedrow

thedrow Jul 28, 2017

Contributor

why are we importing stuff here?

This comment has been minimized.

@georgepsarakis

georgepsarakis Jul 28, 2017

Member

No good reason. Fixed in latest commit, thanks.

@rtpg

This comment has been minimized.

rtpg commented Jul 28, 2017

At least for the changes to now, that seems right to me

@georgepsarakis georgepsarakis merged commit 87b263b into celery:master Jul 28, 2017

3 checks passed

code-quality/landscape Code quality remained the same
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@georgepsarakis georgepsarakis deleted the georgepsarakis:bug/4160_incorrect_timezone_app_now branch Jul 28, 2017

@georgepsarakis

This comment has been minimized.

Member

georgepsarakis commented Jul 28, 2017

Thank you both for the reviews 😄 .

@steve-gregory

This comment has been minimized.

steve-gregory commented Jul 28, 2017

I believe this PR should also close #4145 ?

@marco-silva0000

This comment has been minimized.

marco-silva0000 commented Jul 28, 2017

maybe this is also related #4041

however I think there is still a bug somewhere, but may not be directly related in all issues. please see my comment there with a small explanation

@razerraz

This comment has been minimized.

razerraz commented Aug 7, 2017

It seems this commit don't resolve everything :
celery/django-celery-beat#70 (comment)

pztrick added a commit to pztrick/django-celery-beat that referenced this pull request Aug 8, 2017

Removing code that forced last_run_at to be timezone naive for no rea…
…son, made timezone aware. Fixes crontab schedules after celery/celery#4173.
@nooperpudd

This comment has been minimized.

nooperpudd commented Aug 10, 2017

hopefully release this pull request as soon as possible.

@chuanma

This PR doesn't update automated test. Does Celery have a unit test to guard against this kind of bug?

I just started using Celery, and this bug screwed me a couple of days, wondering why Celery doesn't run tasks at proper time.

@ldsink

This comment has been minimized.

ldsink commented Aug 11, 2017

This PR solve our problems. Please release a fix version for 4.1.x series as soon as possible.

@rtpg

This comment has been minimized.

rtpg commented Aug 14, 2017

@chuanma This does update some unit tests that cover the calculation of "now". Because of the low-level nature of this bug, higher level tests won't catch this bug (Because ultimately a lot of code relied on this code path for determining the time).

When everyone holds the same broken clock, hard to notice that it's broken. Luckily, it also means there's usually just one place to fix

@thedrow thedrow modified the milestone: v4.2 Aug 18, 2017

canassa added a commit to canassa/celery that referenced this pull request Aug 21, 2017

Correct calculation of application current time with timezone (celery…
…#4173)

* Use datetime.astimezone to adjust current time

* Remove import statements from Celery.now

canassa added a commit to canassa/celery that referenced this pull request Aug 21, 2017

Correct calculation of application current time with timezone (celery…
…#4173)

* Use datetime.astimezone to adjust current time

* Remove import statements from Celery.now
@pieterdd

This comment has been minimized.

pieterdd commented Sep 21, 2017

Any chance of a 4.1.1 hotfix release happening? We recently upgraded from v4.0.2 to v4.1, thinking a minor version release from two months ago would have been a safe upgrade. But due to our reliance on crontab for some of our periodic tasks, this is impacting us.

@jobec

This comment has been minimized.

jobec commented Sep 21, 2017

We downgraded to v4.0.2 because of this issue...

@alexmic

This comment has been minimized.

alexmic commented Oct 3, 2017

Same here, 4.1 series broke our scheduling because our timezone is Europe/London.

@JoshYuJump

This comment has been minimized.

JoshYuJump commented Oct 4, 2017

Wondering when to release the PR.

@JoshYuJump

This comment has been minimized.

JoshYuJump commented Oct 20, 2017

hopefully release this pull request as soon as possible.

@fanjindong

This comment has been minimized.

fanjindong commented Oct 20, 2017

@JoshYuJump

This comment has been minimized.

JoshYuJump commented Oct 20, 2017

@fanjindong , I thank the PR has fixed it.

@timosir

This comment has been minimized.

timosir commented Oct 23, 2017

@JoshYuJump
CELERY_TIMEZONE = 'UTC'
CELERY_ENABLE_UTC = True
CELERY_TIMEZONE = 'Asia/Shanghai'
CELERYBEAT_SCHEDULE = {
'cc':{
'task':'operate.check_hangqing_sts',
'schedule': crontab(minute="38",hour='15')
}
}

celery --version
4.1.0 (latentcall)

At this point in my setting, do not work??

@timosir

This comment has been minimized.

timosir commented Oct 23, 2017

TIME_ZONE='Asia/Shanghai'
CELERY_ENABLE_UTC = False
CELERY_TIMEZONE = 'Asia/Shanghai'
CELERYBEAT_SCHEDULE = {
'cc':{
'task':'operate.check_hangqing_sts',
'schedule': crontab(minute="10",hour='14')
}
}

celery --version
4.1.0 (latentcall)

At this point in my setting, do not work??

@fanjindong

This comment has been minimized.

fanjindong commented Oct 23, 2017

JonPeel added a commit to Maplecroft/celery that referenced this pull request Oct 31, 2017

Correct calculation of application current time with timezone (celery…
…#4173)

* Use datetime.astimezone to adjust current time

* Remove import statements from Celery.now

athre0z added a commit to athre0z/celery that referenced this pull request Nov 12, 2017

Correct calculation of application current time with timezone (celery…
…#4173)

* Use datetime.astimezone to adjust current time

* Remove import statements from Celery.now
@jobec

This comment has been minimized.

jobec commented Jan 3, 2018

Any chance of releasing a new version for this any time soon?

@auvipy

This comment has been minimized.

Member

auvipy commented Jan 4, 2018

yes version 4.2 is due in January 2018

@xycloud

This comment has been minimized.

xycloud commented Jan 17, 2018

that is really a super bug....
hope celery can make more test before release

@auvipy

This comment has been minimized.

Member

auvipy commented Jan 17, 2018

@xycloud celery is a volunteer driven project and relies on the effort of the volunteers like you and me. please help with contributions in any form which is useful. we really lack enough resource to manage this huge project.

@xycloud

This comment has been minimized.

xycloud commented Jan 17, 2018

yes, thanks celery sincerely, its really a awesomeness.
maybe celery can accept some donations if possible

@auvipy

This comment has been minimized.

Member

auvipy commented Jan 17, 2018

yes but we had plans for donation but failed to reach our goal.

@browniebroke browniebroke referenced this pull request Feb 2, 2018

Merged

Upgrade celery to 4.2 #1446

pztrick added a commit to pztrick/django-celery-beat that referenced this pull request Feb 9, 2018

Removing code that forced last_run_at to be timezone naive for no rea…
…son, made timezone aware. Fixes crontab schedules after celery/celery#4173.

khyang-udemy added a commit to udemy/celery that referenced this pull request Feb 28, 2018

Correct calculation of application current time with timezone (celery…
…#4173)

* Use datetime.astimezone to adjust current time

* Remove import statements from Celery.now

andrii-sherepa pushed a commit to knowledge-point/celery that referenced this pull request Mar 20, 2018

@jobec

This comment has been minimized.

jobec commented Jun 5, 2018

It looks like this did not make it into version 4.1.1?

@thedrow

This comment has been minimized.

Contributor

thedrow commented Jun 5, 2018

I didn't cherry pick this into 4.1.1.
Use 4.2 when it is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment