Opened 4 years ago

Closed 3 years ago

#14861 closed Bug (fixed)

Importing settings in a module that contains a logging Handler causes circular import.

Reported by: donspaulding Owned by: Claude Paroz <claude@…>
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: percivall@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If settings.py contains the following config for LOGGING:

LOGGING = {
  'version': 1,
  'handlers': {
    'custom_handler': {
      'level': 'INFO',
      'class': 'myproject.logconfig.MyHandler',
    }
  }
}

and myproject/logconfig.py has the following line before the MyHandler definition:

from django.conf import settings

The dictconfig module spits out a none-too-helpful traceback:

Traceback (most recent call last):
  File "manage.py", line 11, in <module>
    execute_manager(settings)
  File "/home/don/myproject/src/django/django/core/management/__init__.py", line 438, in execute_manager
    utility.execute()
  File "/home/don/myproject/src/django/django/core/management/__init__.py", line 379, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/don/myproject/src/django/django/core/management/__init__.py", line 252, in fetch_command
    app_name = get_commands()[subcommand]
  File "/home/don/myproject/src/django/django/core/management/__init__.py", line 101, in get_commands
    apps = settings.INSTALLED_APPS
  File "/home/don/myproject/src/django/django/utils/functional.py", line 276, in __getattr__
    self._setup()
  File "/home/don/myproject/src/django/django/conf/__init__.py", line 41, in _setup
    self._wrapped = Settings(settings_module)
  File "/home/don/myproject/src/django/django/conf/__init__.py", line 126, in __init__
    logging_config_func(self.LOGGING)
  File "/home/don/myproject/src/django/django/utils/dictconfig.py", line 553, in dictConfig
    dictConfigClass(config).configure()
  File "/home/don/myproject/src/django/django/utils/dictconfig.py", line 352, in configure
    '%r: %s' % (name, e))
ValueError: Unable to configure handler 'custom_handler': Unable to configure handler 'custom_handler': 'module' object has no attribute 'logconfig'

The problem is easily rectified once you realize that it's a circular import. Apart from lazily loading the logging config, I'm not even sure there's an appropriate code fix. I'm mainly filing this bug to see if a note can be placed alongside the logging configuration docs that warns of the potential problem.

Attachments (4)

logging-imports.diff (1.3 KB) - added by adamv 4 years ago.
0001-move-logging-setup-to-after-settings-_really_-is-ful.patch (2.2 KB) - added by simonpercivall 3 years ago.
fix circular imports.
14861-3.diff (5.3 KB) - added by claudep 3 years ago.
Move logging config outside of Settings.init
14861-4.diff (5.5 KB) - added by claudep 3 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 4 years ago by russellm

  • Component changed from Uncategorized to Documentation
  • milestone set to 1.3
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

I don't think there's much we can do here beyond having a documentation note warning of the problem.

Changed 4 years ago by adamv

comment:2 Changed 4 years ago by ojii

  • Has patch set

comment:3 Changed 4 years ago by gabrielhurley

  • Resolution set to fixed
  • Status changed from new to closed

In [15449]:

Fixed #14861 -- Added an admonition about the potential for circular imports with custom logging handlers (which produces an extremely confusing error message) to the logging docs. Thanks to donspaulding for the report and adamv for the patch.

comment:4 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Changed 3 years ago by simonpercivall

fix circular imports.

comment:5 Changed 3 years ago by simonpercivall

  • Easy pickings unset
  • Resolution fixed deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Uncategorized
  • UI/UX unset

Actually, moving the logging setup from Settings.__init__ to below self._wrapped = Settings(settings_module) in LazySettings._setup fixes the circular import (at least it fixed my problem). See the attached patch.

comment:6 Changed 3 years ago by claudep

  • Component changed from Documentation to Core (Other)

comment:7 Changed 3 years ago by simonpercivall

Ping.

Sorry for the no-content comment, but this is still very much a problem, and it's a simple fix, and it comes with a patch.

comment:8 Changed 3 years ago by claudep

Could you check if you can still reproduce with latest development code? And then, a sample project to demonstrate that your patch is solving the issue would be appreciated.

comment:9 Changed 3 years ago by simonpercivall

I thought I would get notified by just commenting. Apparently not.

It's still reproducible with 1.5 HEAD.

A sample project? How? A zip? Sure, if it's necessary.

It's pretty easy to see, however, by just looking at the code, that referencing settings from the logging config, directly or by importing a module that does, before _wrapped is set will cause infinite recursion. And since my patch moves logging setup to after _wrapped is set, the issue is resolved.

comment:10 Changed 3 years ago by claudep

  • Owner nobody deleted
  • Status changed from reopened to new
  • Type changed from Uncategorized to Bug

OK, no need for any more examples, I've been able to create a test case to reproduce the issue. I will propose a patch soon (including yours).

I thought I would get notified by just commenting. Apparently not.

You need to put yourself in the cc field to get notified.

comment:11 Changed 3 years ago by simonpercivall

  • Cc percivall@… added

comment:12 Changed 3 years ago by claudep

Attached patch containing simonpercivall's patch, test, and removal of [66312066a01af1325280b07d8e6942b03a46d650].

Changed 3 years ago by claudep

Move logging config outside of Settings.init

comment:13 Changed 3 years ago by james@…

What if my log handler wants to use the models? by declaring an import of models will cause circular import of settings.

Changed 3 years ago by claudep

comment:14 Changed 3 years ago by claudep

I've just updated my patch, where I confined the logging setup in a private method and I also let a potential circular import warning in the documentation.

comment:15 Changed 3 years ago by anonymous

I don't think there's a risk of circular import here. Settings are fully configured, the logging setup in itself has no dependency on models. Where would the risk come from?

comment:16 Changed 3 years ago by claudep

OK, I can commit it without the admonition, then we'll see if any further issue is raised, we can always add it later.

comment:17 Changed 3 years ago by Claude Paroz <claude@…>

  • Owner set to Claude Paroz <claude@…>
  • Resolution set to fixed
  • Status changed from new to closed

In fc69fff9ab5bba8a6cff3eaf51f02a3204b1c015:

Fixed #14861 -- Moved logging config outside of Settings.init

Thanks donspaulding for the report and simonpercivall for the
initial patch.

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