[Django] #32708: Django cron file lock breaks with django 3.2
Django
-------------------------------------+-------------------------------------
Reporter: daillouf | Owner: nobody
Type: Bug | Status: new
Component: Core | Version: 3.2
(Other) | Keywords: file lock, too many
Severity: Normal | connections, django_cron
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.
I believe that lock function is not documented though, so it's up to you
to decide,
whether it's Django or Django_cron that needs fixing.
have a good day !
--
Ticket URL: <https://code.djangoproject.com/ticket/32708>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: daillouf | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: file lock, too many | Triage Stage:
connections, django_cron | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> }}}
>
New description:
Hello everyone !
{{{
}}}
Edit: that problem was spotted at code review but ignored :
[ https://github.com/django/django/pull/13410#discussion_r624988346]
have a good day !
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32708#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: daillouf | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: file lock, too many | Triage Stage:
connections, django_cron | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by daillouf):
related ticket introducing the breaking change
[https://code.djangoproject.com/ticket/31989]
--
Ticket URL: <https://code.djangoproject.com/ticket/32708#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: daillouf | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: file lock, too many | Triage Stage:
connections, django_cron | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Hasan Ramezani (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32708#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution: invalid
Keywords: file lock, too many | Triage Stage:
connections, django_cron | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
* component: Core (Other) => File uploads/storage
Comment:
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32708#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution:
connections, django_cron | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: invalid =>
Comment:
oh sorry, my bad, I didn't the full MR.
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
--
Ticket URL: <https://code.djangoproject.com/ticket/32708#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: File | Version: 3.2
uploads/storage |
Keywords: file lock, too many | Triage Stage:
connections, django_cron | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
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.
[https://github.com/Tivix/django-cron/commits/master 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32708#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution: invalid
Keywords: file lock, too many | Triage Stage:
connections, django_cron | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by François Dailloux):
Replying to [comment:6 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.
>
> [https://github.com/Tivix/django-cron/commits/master 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
--
Ticket URL: <https://code.djangoproject.com/ticket/32708#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution: invalid
Keywords: file lock, too many | Triage Stage:
connections, django_cron | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
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/
--
Ticket URL: <https://code.djangoproject.com/ticket/32708#comment:8>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution: invalid
Keywords: file lock, too many | Triage Stage:
connections, django_cron | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:8 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32708#comment:9>