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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move signal import to django.core #1357

Merged
merged 3 commits into from Nov 11, 2023

Conversation

AlanCoding
Copy link
Contributor

@AlanCoding AlanCoding commented Nov 4, 2023

Description of the Change

Hi 馃憢 , 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: unbit/uwsgi#1969 (comment)

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

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@AlanCoding
Copy link
Contributor Author

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 django/django@5dddd79

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

@AlanCoding
Copy link
Contributor Author

It turns out that the change was already adopted into Django Rest Framework here encode/django-rest-framework#8699

@dopry
Copy link
Contributor

dopry commented Nov 10, 2023

@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?

Copy link

codecov bot commented Nov 10, 2023

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%> (酶)

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

@AlanCoding AlanCoding marked this pull request as ready for review November 10, 2023 18:37
Copy link
Contributor

@dopry dopry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Your contributIon is appreciated!

@dopry dopry merged commit 862cb7a into jazzband:master Nov 11, 2023
27 checks passed
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 this pull request may close these issues.

None yet

2 participants