Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#32708 closed Bug (invalid)

Django cron file lock breaks with django 3.2

Reported by: François Dailloux Owned by: nobody
Component: File uploads/storage Version: 3.2
Severity: Normal Keywords: file lock, too many connections, django_cron
Cc: Hasan Ramezani Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by François Dailloux)

Hello everyone !

After I upgraded django to 3.2, I noticed that I had the following error popping up

django.db.utils.OperationalError: FATAL: sorry, too many clients already

I then inspected the postgresql connections, and my processes, and it turned out, that was my django cron jobs that were running multiple times at the same time, thus opening more connections than necessary.

[Django cron has a lock mecanism to avoid this,](https://github.com/Tivix/django-cron/blob/v0.5.1/django_cron/backends/lock/file.py) that uses the lock function from django :
from django.core.files import locks

But in django3.2 with latest django_cron version, that check totally fails and we can run a cron multiple times. I looked up a bit the code to see what's the issue.

In django 3.2 [the code for that lock function is ](https://github.com/django/django/blob/main/django/core/files/locks.py)

        def lock(f, flags):
            try:
                fcntl.flock(_fd(f), flags)
                return True
            except BlockingIOError:
                return False

        def unlock(f):
            fcntl.flock(_fd(f), fcntl.LOCK_UN)
            return True

So BlockingIOError are catched, and the function then returns false.

[This changes the behavior from 3.1.7 : ](https://github.com/django/django/blob/stable/3.1.x/django/core/files/locks.py)

        def lock(f, flags):
            ret = fcntl.flock(_fd(f), flags)
            return ret == 0

        def unlock(f):
            ret = fcntl.flock(_fd(f), fcntl.LOCK_UN)
            return ret == 0


in previous behaviour, checking for «ret==0» was indeed useless, because when flock fails, it raises a IOError,

But in 3.1, the lock function didn't catch the error, and django_cron made good use of that behaviour.

Edit: that problem was spotted at code review but ignored :

[ https://github.com/django/django/pull/13410#discussion_r624988346]

have a good day !

Change History (9)

comment:1 by François Dailloux, 3 years ago

Description: modified (diff)

comment:2 by François Dailloux, 3 years ago

related ticket introducing the breaking change

https://code.djangoproject.com/ticket/31989

comment:3 by Hasan Ramezani, 3 years ago

Cc: Hasan Ramezani added

comment:4 by Mariusz Felisiak, 3 years ago

Component: Core (Other)File uploads/storage
Resolution: invalid
Status: newclosed

Edit: that problem was spotted at code review but ignored :

That's not true, we discussed possible options and decided to catch only BlockingIOError. Moreover it's a change in the private API that was documented in the release notes. You should report an issue in the django-cron bugtracker.

comment:5 by François Dailloux, 3 years ago

Resolution: invalid
Status: closednew

oh sorry, my bad, I didn't read the full MR and missed that part

Yes, in an ideal world, we could fix django cron.

if django cron was actually an active project…

the latest release of django-cron was 2018, so i'm afraid nothing will happen there.

Do you have an internal policy about breaking changes ? Shouldn't backwards-breaking changes be made in major versions upgrades ?

Thank you for reading me

Last edited 3 years ago by François Dailloux (previous) (diff)

comment:6 by Mariusz Felisiak, 3 years ago

Resolution: invalid
Status: newclosed

Please don't reopen closed tickets.

Do you have an internal policy about breaking changes ? Shouldn't backwards-breaking changes be made in major versions upgrades?

3.2 is a major versions, and it's hard to say it's a breaking change because this is a private API.

if django cron was actually an active project…
the latest release of django-cron was 2018, so i'm afraid nothing will happen there.

The last commit is 5 days ago so it doesn't look unmaintained. Again, you should try to report this issue in the django-cron bugtracker.

in reply to:  6 comment:7 by François Dailloux, 3 years ago

Replying to Mariusz Felisiak:

Please don't reopen closed tickets.

Do you have an internal policy about breaking changes ? Shouldn't backwards-breaking changes be made in major versions upgrades?

3.2 is a major versions, and it's hard to say it's a breaking change because this is a private API.

if django cron was actually an active project…
the latest release of django-cron was 2018, so i'm afraid nothing will happen there.

The last commit is 5 days ago so it doesn't look unmaintained. Again, you should try to report this issue in the django-cron bugtracker.

Ok well I won't re-open it then, I just felt it was better to keep all the history of this ticket here.

indeed that's a private API, django_cron shouldn't have used it at all in the first place

Django_cron still has no release since 3 years, the company tivix probably updates the repo for their own usage, but you're right, it's worth a try

have a good day

in reply to:  6 ; comment:8 by François Dailloux, 3 years ago

I've just created an issue in django_cron https://github.com/Tivix/django-cron/issues/169

3.2 is a major versions, and it's hard to say it's a breaking change because this is a private API.

Is it documented anywhere that this locks module is private ?

it's not the whole core module, is it ? I've seen that both django cron and DRF use functions from the core module

on a side note, version 3.1.7 in semver, 3 is the major, 1 the minor, 7 the patch. It confused me as well that the minor wasn't the right-most number.

Semver is overrated anyway, any change will break something somewhere https://xkcd.com/1172/

in reply to:  8 comment:9 by Mariusz Felisiak, 3 years ago

Replying to François Dailloux:

I've just created an issue in django_cron https://github.com/Tivix/django-cron/issues/169

3.2 is a major versions, and it's hard to say it's a breaking change because this is a private API.

Is it documented anywhere that this locks module is private ?

it's not the whole core module, is it ? I've seen that both django cron and DRF use functions from the core module

Anything that is not documented is considered a private API, see https://docs.djangoproject.com/en/stable/misc/api-stability/#api-stability.

on a side note, version 3.1.7 in semver, 3 is the major, 1 the minor, 7 the patch. It confused me as well that the minor wasn't the right-most number.

Semver is overrated anyway, any change will break something somewhere https://xkcd.com/1172/

See https://docs.djangoproject.com/en/stable/internals/release-process/#release-cadence.

Please use support channels if you have further questions.

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