Skip to content
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

Fix more logger related memory leaks on Windows #29957

Merged
merged 1 commit into from
Dec 29, 2015

Conversation

skizunov
Copy link
Contributor

Some observations have been made on Windows:

  • Any process (besides the main process) that does not call
    salt.log.setup.setup_multiprocessing_logging() will leak. The reason is
    that because Windows doesn't copy process state when spawning processes,
    each new process will have the temp null and temp queue log handlers
    installed (the default state in salt/log/setup.py when initializing).
    Those handlers just store stuff and hence will use much memory.
  • It was obversed that for some of the classes, register_after_fork()
    wouldn't work and so salt.log.setup.setup_multiprocessing_logging()
    wouldn't be invoked causing leaks. However, for some of the classes,
    register_after_fork() would work. After investigation, it was determined
    that register_after_fork() worked only on processes that used
    __setstate__ / __getstate__ and hence would invoke __init__() inside
    the child process. Thus, register_after_fork() only seems to work on
    Windows when invoked from within the child process, but not the parent
    process, making it not very useful (since if we are in the child process,
    we can invoke salt.log.setup.setup_multiprocessing_logging() directly).

The following changes have been made based on these observations:

  • Try to make almost every process use or inherit from
    MultiprocessingProcess so as to call
    salt.log.setup.setup_multiprocessing_logging().
  • Processes that don't use or inherit from MultiprocessingProcess invoke
    salt.log.setup.setup_multiprocessing_logging() themselves.
  • Try to make every process that uses or inherits from
    MultiprocessingProcess invoke __init__ from the child process.
    In order to accomplish this, all such processes use __setstate__ and
    __getstate__.
  • When multiprocessing.Process is constructed via __init__ in the child
    process, certain features such as is_alive() and ident() don't work.
    Thus we set self._is_child = True in each __setstate__ to be able to
    distinguish whether the __init__ invocation is from the parent or child
    process.

Signed-off-by: Sergey Kizunov sergey.kizunov@ni.com

Some observations have been made on Windows:
- Any process (besides the main process) that does not call
`salt.log.setup.setup_multiprocessing_logging()` will leak. The reason is
that because Windows doesn't copy process state when spawning processes,
each new process will have the temp null and temp queue log handlers
installed (the default state in `salt/log/setup.py` when initializing).
Those handlers just store stuff and hence will use much memory.
- It was obversed that for some of the classes, `register_after_fork()`
wouldn't work and so `salt.log.setup.setup_multiprocessing_logging()`
wouldn't be invoked causing leaks. However, for some of the classes,
`register_after_fork()` would work. After investigation, it was determined
that `register_after_fork()` worked only on processes that used
`__setstate__` / `__getstate__` and hence would invoke `__init__()` inside
the child process. Thus, `register_after_fork()` only seems to work on
Windows when invoked from within the child process, but not the parent
process, making it not very useful (since if we are in the child process,
we can invoke `salt.log.setup.setup_multiprocessing_logging()` directly).

The following changes have been made based on these observations:
- Try to make almost every process use or inherit from
`MultiprocessingProcess` so as to call
`salt.log.setup.setup_multiprocessing_logging()`.
- Processes that don't use or inherit from `MultiprocessingProcess` invoke
`salt.log.setup.setup_multiprocessing_logging()` themselves.
- Try to make every process that uses or inherits from
`MultiprocessingProcess` invoke `__init__` from the child process.
In order to accomplish this, all such processes use `__setstate__` and
`__getstate__`.
- When `multiprocessing.Process` is constructed via `__init__` in the child
process, certain features such as `is_alive()` and `ident()` don't work.
Thus we set `self._is_child = True` in each `__setstate__` to be able to
distinguish whether the `__init__` invocation is from the parent or child
process.

Signed-off-by: Sergey Kizunov <sergey.kizunov@ni.com>
@cachedout
Copy link
Contributor

This looks totally sensible to me. Nice work, @skizunov

I would also like to run this past @s0undt3ch before we merge this.

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Dec 22, 2015
@s0undt3ch
Copy link
Collaborator

This seems pretty reasonable to me and an excellent work from @skizunov.

Let's just make sure that all of salt's daemons exit properly on CTRL-C and when signalled with SIG_TERM and SIG_INT, that's my only concern.

Thank You @skizunov!

cachedout pushed a commit that referenced this pull request Dec 29, 2015
Fix more logger related memory leaks on Windows
@cachedout cachedout merged commit 0393b15 into saltstack:develop Dec 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants