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

@ignore_database_error might be a code smell #79

Closed
jcass77 opened this issue Jul 15, 2020 · 0 comments · Fixed by #89
Closed

@ignore_database_error might be a code smell #79

jcass77 opened this issue Jul 15, 2020 · 0 comments · Fixed by #89

Comments

@jcass77
Copy link
Owner

jcass77 commented Jul 15, 2020

The jobstores.ignore_database_error decorator is applied to various DjangoJobStore methods that interact with the database, and seems to quietly mute a whole range of database-related exceptions.

It looks like this was intended to catch a specific use case to prevent someone from attempting to use django-apscheduler without first applying the Django database migrations, but an unfortunate side-effect is that this decorator can also cause the package to break in unpredictable ways.

An example of a specific issue that I have encountered is outlined below:

scheduler = BlockingScheduler()
scheduler.add_jobstore(DjangoJobStore(), "default")

scheduler.add_job(
    my_job,
    trigger=CronTrigger(minute="*/10"),  # Every 10 mins
    id="my_job",
    max_instances=1,
    replace_existing=True,
)

If add_job fails because an exception is raised while attempting to create the DjangoJob instance, then:

  • @ignore_database_error will mute the exception
  • the DjangoJob instance will never be created; and
  • the job will not be scheduled; but
  • scheduler will be unaware of the failure, and give no obvious indication that a problem has occurred apart from logging a warning.

This has the unpleasant consequence that errors might go undetected in a production environment for quite a while before anyone notices that the jobs are not being executed anymore.

It might be preferable for the exceptions to be raised so that the user can decide how best to deal with them - unless I am missing something obvious?

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 a pull request may close this issue.

1 participant