Code

Opened 6 years ago

Closed 3 months ago

#8540 closed Bug (wontfix)

daemonize should close all open files

Reported by: paulegan Owned by: nobody
Component: Core (Management commands) Version: master
Severity: Normal Keywords: daemonize become_daemon fastcgi
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In source:django/utils/daemonize.py, the become_daemon function should close all open file descriptors, not just the standard three. Without this, a fastcgi process can hold open files or sockets from the parent process.

Attachments (1)

daemonize.diff (617 bytes) - added by paulegan 6 years ago.
Patch for become_daemon to close all open files

Download all attachments as: .zip

Change History (15)

Changed 6 years ago by paulegan

Patch for become_daemon to close all open files

comment:1 Changed 6 years ago by mtredinnick

  • milestone set to 1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Good practice, although there's no sensible way of running that program which will have extra open file-descriptors, which is why it isn't doing that already. Won't hurt to protect people from themselves, though.

comment:2 Changed 6 years ago by mtredinnick

  • Triage Stage changed from Accepted to Ready for checkin

comment:3 Changed 6 years ago by jacob

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

os.sysconf isn't available on windows, so this needs to work around that somehow.

comment:4 Changed 6 years ago by mtredinnick

  • milestone changed from 1.0 to post-1.0

Pushing to after 1.0. Don't know what I was thinking this morning when I set the milestone here. If we get this wrong, it destabilises things.

comment:5 Changed 6 years ago by paulegan

The [source:django/trunk/django/utils/daemonize.py become_daemon] function has two definitions, switched on os.name. The change only applies to the os.name == 'posix' version, for which os.sysconf should always be defined.

I don't normally use Windows, but I quickly checked: the native python install on Windows has os.name == 'nt', and cygwin python (and other posix-based ports) do have os.sysconf defined.

comment:6 Changed 6 years ago by dcwatson

I also ran into this issue when spawning a daemon from a view to build some reports. Basically, the view called a script using os.system, and the script set DJANGO_SETTINGS_MODULE and did some processing using django, after calling become_daemon. The os.system call would return immediately, but it looked like django would not finish it's request processing until after the spawned script finished. Applying this patch fixed the problem.

I don't fully understand the black art of daemonization, but it seems like this may not even be an issue on Windows. Nothing is being forked, there is no "parent process", it's just redirecting stdin, stdout, and stderr. Regardless, it seems like fixing a bug for posix systems, even if still exists on Windows, is better for 1.0 than leaving the bug for both.

comment:7 Changed 6 years ago by mtredinnick

@dcwatson: yours is a case of using our internal daemonisation process for external usage. That's definitely a situation where closing the external descriptors would be necessary. We've written that thing for internal usage only, but once again people have fallen for the "everything in django.utils.* is intended for public reuse trap (it's not).

This is a case where Django really shouldn't be supplying that stuff for public reuse; it's not our domain. We're providing a web/database framework, not something that provides extra utilities like daemonising functionality -- those are better provided by other frameworks. This patch would help bring django.utils.daemonise up to that level of utility, but for 1.0 that isn't a goal. The patch in some form will go in post-1.0, but it's not a necessary bug fix at the moment (and the API stability document for 1.0 will make it clear what portions of django.utils are available for public usage.

I realise it looks like a simple case and what could possibly go wrong? But it's a feature addition: there is no bug in the way this thing is used in Django and it is not intended as public API.

comment:8 Changed 6 years ago by dcwatson

Fair enough. I agree, if it's not meant for public consumption, no need to fix it for yahoos like me using it publicly :-) Looking forward to the API stability document. I admit I was thrown by the docstring on the function -- "Robustly turn into a UNIX daemon..." Is it worth documenting these types of things as non-public in code as well as the API stability doc?

comment:9 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:10 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 13 months ago by aaugustin

  • Component changed from Core (Other) to Core (Management commands)

comment:14 Changed 3 months ago by timo

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

runfcgi is deprecated in Django 1.7 and that appears to be the only code in Django that uses django/utils/daemonize.py.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.