Opened 8 years ago

Closed 7 years ago

#6175 closed (fixed)

become_daemon should use os._exit instead of sys.exit

Reported by: tclugg@… Owned by: Leo
Component: Core (Other) Version: master
Severity: Keywords: daemon, daemonize, become_daemon, segfault
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

django.utils.daemonize.become_daemon forks then uses sys.exit(0) instead of os._exit(0), which can cause some nasty issues. os._exit is the correct way to exit after forking in Python, as os._exit avoids calling Python cleanup functions which may cause issues such as segfaults.

See http://docs.python.org/lib/os-process.html for details on os._exit().

Attachments (2)

daemon.patch (793 bytes) - added by Tyson Clugg <tclugg@…> 8 years ago.
6175_second_patch.diff (477 bytes) - added by Leo 7 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Tyson Clugg <tclugg@…>

comment:1 Changed 8 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from become_daemon should use os._exit instead of sys.exit to [patch] become_daemon should use os._exit instead of sys.exit

comment:2 Changed 7 years ago by SmileyChris

  • Summary changed from [patch] become_daemon should use os._exit instead of sys.exit to become_daemon should use os._exit instead of sys.exit
  • Triage Stage changed from Unreviewed to Design decision needed

Pushing to a design decision as it's over my head :)

comment:3 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

This is only half right. The main parent should call sys.exit(), since it can be cleaned up normally; it's a normal short-lived process. The children should generally call os._exit() to avoid double cleanups via atexit() paths.

So I believe the first change is wrong and the second change is right, but I need to do some re-reading to confirm this.

Changed 7 years ago by Leo

comment:4 Changed 7 years ago by Leo

  • Owner changed from nobody to Leo
  • Status changed from new to assigned

Malcolm, you're right. The first exit() call is in the parent so it should stay exit(). The second call is in the first child, so _exit() is more appropriate. I've attached a patch only to correct the second one.

comment:5 Changed 7 years ago by mtredinnick

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

(In [7290]) Fixed #6175 -- Use os._exit() instead of sys.exit() in child processes. Based
on a patch from Tyson Clugg.

comment:6 Changed 7 years ago by durdinator

  • Resolution fixed deleted
  • Status changed from closed to reopened

Malcolm wrote:

So I believe the first change is wrong and the second change is right, but I need to do some re-reading to confirm this.

and Leo wrote:

Malcolm, you're right. The first exit() call is in the parent so it should stay exit().

But the patch actually committed changes both calls. Are you sure that's correct?

comment:7 Changed 7 years ago by mtredinnick

No it didn't. Have a look at the line numbers affected. The patch I committed was a variant on the two attached to these tickets and fixed a place that was overlooked in both those patches.

comment:8 Changed 7 years ago by durdinator

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

Ah, I see. Glad to see it wasn't a mistake after all.

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