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

Feature: simplified closing of old db connections #170

Open
HeyHugo opened this issue Jul 16, 2022 · 4 comments
Open

Feature: simplified closing of old db connections #170

HeyHugo opened this issue Jul 16, 2022 · 4 comments

Comments

@HeyHugo
Copy link

HeyHugo commented Jul 16, 2022

I'm using a customised BlockingScheduler class to ensure db connections are closed before/after running tasks.

This simplifies things for me in two ways:

  • I don't have to add the close_old_connections decorator to my tasks
  • I don't have to mock the close_old_connections decorator otherwise breaking my tests by prematurely closing the db connection my test session is using.

It looks like this

import functools
from apscheduler.schedulers.blocking import BlockingScheduler
from django import db

class BlockingDBScheduler(BlockingScheduler):
    def add_job(self, func, *args, **kwargs):
        @functools.wraps(func)
        def wrapper():
            db.close_old_connections()
            try:
                result = func(*args, **kwargs)
            finally:
                db.close_old_connections()
            return result

        return super().add_job(wrapper, *args, **kwargs)

If you'd think this class would fit as a part of the package I could make a PR, or if you think it'd be a nice addition to the docs as an alternative to the @close_old_connections-decorator I look at that as well.

@jcass77
Copy link
Owner

jcass77 commented Jul 22, 2022

Would this achieve the same result and perhaps be easier to understand (untested)?

import functools
from apscheduler.schedulers.blocking import BlockingScheduler
from django import db

class BlockingDBScheduler(BlockingScheduler):
    @util.close_old_connections
    def add_job(self, func, *args, **kwargs):
        # Edited to swap `wrapper` with `func` as per the correction pointed out by @HeyHugo in the comments below.
        return super().add_job(func, *args, **kwargs)

@HeyHugo
Copy link
Author

HeyHugo commented Jul 22, 2022

@jcass77 Aha nice yes that works (just swap wrapper arg to func)

@jcass77
Copy link
Owner

jcass77 commented Jul 23, 2022

Ok. I think this approach could be handy for folks who only intend to schedule jobs that always read or mutate the database.

The drawback is that if you use something like BlockingDBScheduler for anything else then those extra calls to db.close_old_connections() are essentially superfluous and just unnecessary overhead. That feels like an unreasonable tradeoff for the sake of convenience and syntactical sugar, and maybe not something that we want as the default behaviour for new users.

I'm happy to leave this here for others to find and use in their projects if their use case is narrowly defined along the lines discussed above though.

Thanks for pointing this out and providing a coding example!

@HeyHugo
Copy link
Author

HeyHugo commented Jul 23, 2022

Ok that's fair and I agree.

I have not yet had a case with a scheduled task without db interaction, but I understand there could be such use cases where state lives elsewhere.

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

No branches or pull requests

2 participants