Skip to content

Conversation

@AlanCoding
Copy link
Contributor

@AlanCodingAlanCoding 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
ContributorAuthor

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
ContributorAuthor

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

@dopry
Copy link
Member

@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
Copy link

codecovbot 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 
FilesCoverage Δ
oauth2_provider/settings.py100.00% <100.00%> (ø)

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

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

@doprydopry 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!

@doprydopryforce-pushed the core_setting_change branch from a891421 to aa37593CompareNovember 11, 2023 20:50
@doprydopry merged commit 862cb7a into django-oauth:masterNov 11, 2023
Sign up for freeto 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.

2 participants

@AlanCoding@dopry