jazzband / django-oauth-toolkit

OAuth2 goodies for the Djangonauts!
https://django-oauth-toolkit.readthedocs.io
Other
3.13k stars 792 forks source link

Move signal import to django.core #1357

Closed AlanCoding closed 10 months ago

AlanCoding commented 11 months ago

Description of the Change

Hi :wave: , I believe this is causing us a problem, but I lack technical rigor to be able to explain it fully. By applying this change under certain circumstances, I seem to avoid a core dump that happens as uWSGI recycles workers when it is running in the default prefork mode. The most relevant description of the series of events is this comment: https://github.com/unbit/uwsgi/issues/1969#issuecomment-462252471

Here, I just wish to present this as an improvement to follow best practice. In the django.test.signals module, you can see it imports from this modification with no modifications.

https://github.com/django/django/blob/main/django/test/signals.py

However, somewhat obviously... if you import this test module, you will also register all the test signal connections. Now, that shouldn't do anything because the setting_changed signal is documented to only fire when running tests. However, it also pulls in a lot of other imports which just aren't needed, and as this happens in settings, it triggers fairly early in app load order.

This argument also applies to 1 import in DRF, which I also hope to submit a patch for. I'm still working out final testing on my side, so consider this a draft as of opening.

Checklist

AlanCoding commented 11 months ago

For some more background - here is an issue where they requested to move this signal out of tests https://code.djangoproject.com/ticket/20349

And in 2014 it was implemented in https://github.com/django/django/commit/5dddd79433ceb88ab67d9851b49a44ce5b8f509c

Nonetheless it seems plausible that libraries still have the old import, and that it has even proliferated.

AlanCoding commented 11 months ago

It turns out that the change was already adopted into Django Rest Framework here https://github.com/encode/django-rest-framework/pull/8699

dopry commented 10 months ago

@AlanCoding this looks like a reasonable change. I don't think additional unit tests are needed here as long as tests continue passing. Can you update your branch to add a note to the CHANGELOG.md and add yourself to AUTHORS?

codecov[bot] commented 10 months ago

Codecov Report

Merging #1357 (aa37593) into master (a30001f) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1357   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files          32       32           
  Lines        2120     2120           
=======================================
  Hits         2068     2068           
  Misses         52       52           
Files Coverage Δ
oauth2_provider/settings.py 100.00% <100.00%> (ø)

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!