Opened 3 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#36633 closed Bug (wontfix)

TaskBackend run_after validation assumes datetime when USE_TZ=True

Reported by: Chris M Owned by:
Component: Tasks Version: 6.0
Severity: Release blocker Keywords:
Cc: Jake Howard Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I’ve been working on an implementation of a TaskBackend using Google Cloud Tasks. I ran across a bit of confusion regarding the run_after parameter validation.

The DEP originally referenced that the value could be a datetime or a timedelta. The released implementation looks like it only documents it as a datetime, so I’m not sure if it was meant to support timedelta. However, I tried using a timedelta with the USE_TZ=True and the validation in BaseTaskBackend assumes that the run_after value is a datetime and I got an error

Calling code: my_task.using(run_after=timedelta(seconds=10)).enqueue()

Error: AttributeError: 'datetime.timedelta' object has no attribute 'utcoffset'

If I run the code using a timedelta and with USE_TZ=False then the error isn't thrown and my backend can process it.

The code throwing the error in the BaseTaskBackend is

        if (
            settings.USE_TZ
            and task.run_after is not None
            and not timezone.is_aware(task.run_after)
        ):
            raise InvalidTask("run_after must be an aware datetime.")

I think it would be best for this condition to verify that the provided run_after is a datetime before calling timezone.is_aware. I'm hoping for some confirmation if this is the right way to proceed.

I think it would be great to support timedelta directly as well by updating the type annotations on Task, but I couldn't determine through the PR history if this was removed during implementation.

Change History (5)

comment:1 by Jacob Walls, 3 weeks ago

Cc: Jake Howard added
Severity: NormalRelease blocker
Type: UncategorizedBug

comment:2 by Mariusz Felisiak, 3 weeks ago

Triage Stage: UnreviewedAccepted

comment:3 by Jake Howard, 3 weeks ago

Resolution: wontfix
Status: newclosed

I believe this is behaving as intended. The timedelta functionality was removed post-DEP, because when interacting with the previous transaction handling, it wasn't obvious if it was "10 seconds after enqueue" or "10 seconds from .enqueue call". We could add a type guard that only a datetime is passed in, but given an exception is already raised, rather than strange behaviour, I suspect it's fine.

comment:4 by Mariusz Felisiak, 3 weeks ago

Triage Stage: AcceptedUnreviewed

in reply to:  3 comment:5 by Chris M, 3 weeks ago

Thanks for the context. I figured there was probably a conversation about the timedelta that I was just missing. Personally, I think it is more confusing to have the time be static, when the transaction close time is dynamic, but I understand needing to make a call and go with it, not a problem.

If you are open to it, Jake, I'm happy to cut a PR against django-tasks for the type check before the timezone aware check. You are correct that it is failing, which is the final outcome either way. However, as a developer interacting with it, I was confused about what steps I should take, since the error had less to do with making the value timezone aware and more that I just passed in the wrong type completely. I think the clean up here could create a better developer experience.

Note: See TracTickets for help on using tickets.
Back to Top