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

Timeout on enqueue not working #117

Open
WillNye opened this issue Apr 26, 2016 · 5 comments
Open

Timeout on enqueue not working #117

WillNye opened this issue Apr 26, 2016 · 5 comments

Comments

@WillNye
Copy link

WillNye commented Apr 26, 2016

I did see #49 but since it is almost 2 years old I figured I'd create a new one.
I attempted setting a timeout with the following:

my_scheduler = Scheduler('my_task', connection=Redis())

my_scheduler.enqueue(
a_param,
another_param,
**{'timeout': 2700}
)

While this doesn't fail out it still defaults to 3 minutes.

I've tracked down what I believe to be the issue.

enqueue_in calls _create_job and passes in any kwargs.
_create_job then calls create.
create does not use kwargs to get the timeout, it defines it within the function arguments.

enqueue_in either needs to have a timeout argument or this should be in _create_job

try:
timeout = kwargs['timeout']
except KeyError:
timeout = None

I created a fork which features this at https://github.com/SiSTeRTech/rq-scheduler if you'd like to merge

@lechup
Copy link
Contributor

lechup commented May 5, 2016

I think _create_job should have timeout kwarg, and we should get rid of:

        if timeout is not None:
            job.timeout = timeout

From Scheduler.cron() and other places, just pass it to _create_job.

@selwin is there any reason it's not implemented like that?

@selwin
Copy link
Contributor

selwin commented May 5, 2016

scheduler.schedule already accepts timeout argument, so it would make sense that scheduler.enqueue_in and scheduler.enqueue_at also accept timeout argument.

@lechup I agree with your suggestion, _create_job should accept a timeout argument. PR for this is welcome :).

@lechup
Copy link
Contributor

lechup commented May 6, 2016

@selwin I've created #124 , @WillNye maybe You can base on it?

@lechup
Copy link
Contributor

lechup commented May 6, 2016

@WillNye I've checked Your fork, and it contains more than this change (some changes in setup.py). Can You prepare different branch based on my branch I want to merge in #124 and simply include only changes to enqueue_in and enqueue_at?

@djmaze
Copy link

djmaze commented Jun 3, 2018

Is this problem fixed, as #124 has been merged for a long time now?

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

No branches or pull requests

4 participants