Skip to content

Disable Logging panel when DEBUG is set to False to prevent memory leaks #563

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from

Conversation

stanhu
Copy link

@stanhu stanhu commented Mar 16, 2014

I upgraded django-debug-toolbar from 0.9.4. to 1.0.1, and I noticed my application was quickly leaking memory even when settings.DEBUG was set to False. Once I removed debug_toolbar from the list of applications, the problem disappeared.

Upon further investigation, I found that LoggingPanel was quietly collecting all my log messages and never releasing them.

This problem only occurs if the Django application is fully initialized (e.g. using runserver), since this causes the LoggingPanel to register its handlers. Here is a simple application that illustrates the problem:

from django.core.management import ManagementUtility
import logging

utility = ManagementUtility()
command = utility.fetch_command('runserver')
command.validate()

logger = logging.getLogger('test')

while True:
    logger.info('This logger will leak memory')

Run this application with the default debug_toolbar settings, and then use 'top' to watch the memory grow without bound. The problem appears that the LoggingPanel does not use the enable_instrumentation() function normally called by the middleware, which does the DEBUG check beforehand.

I think the simplest fix is to check if DEBUG is enabled before initializing the LogCollector. This pull request does just that, and it appears to fix the issue.

In addition, I think we should consider a few other things:

  1. Warn the user that under DEBUG mode, memory used for log messages can grow without bounds (this might be true for other panels already).

  2. Limit the number/age of messages stored (perhaps as a configuration option).

@aaugustin aaugustin added the Bug label Mar 25, 2014
@aaugustin
Copy link
Contributor

I think we have to bite the bullet and write a logging.Filter that checks a thread-local flag set by enable/disable_instrumentation if we want to avoid running into further problems.

@camilonova
Copy link
Contributor

@aaugustin can you please review the changes?

Copy link
Contributor

@aaugustin aaugustin left a comment

Choose a reason for hiding this comment

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

Just one small comment.

@@ -15,13 +16,33 @@

class LogCollector(ThreadCollector):

def __init__(self):
ThreadCollector.__init__(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason for using ThreadCollecter instead of super()? If so, I think it should be documented.

Copy link

@RDIL RDIL left a comment

Choose a reason for hiding this comment

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

Need to resolve conflicts.

@auvipy auvipy self-assigned this May 2, 2019
@auvipy auvipy removed their assignment Feb 3, 2021
Base automatically changed from master to main February 11, 2021 15:01
Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

this needs to fix the merge conflicts, or not sure if the fixes are already into main branch?

@matthiask
Copy link
Member

Closing because of inactivity. Please resubmit if you still want to work on this.

@matthiask matthiask closed this Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants