Skip to content

Commit 22ded2c

Browse files
authored
docs(uwsgi): update docs on --lazy-apps for uwsgi and tests (#13550)
**tl;dr** 1. Use `uwsgi>=2.0.30` which has unbit/uwsgi#2726 2. When using `uwsgi<2.0.30` with `--lazy-apps`, also set `--skip-atexit`, if you see crashes when workers exit. ----------------------------------------------------- The following explanation happens on `uwsgi<2.0.30` When `uwsgi` is configured with `--lazy-apps`, master doesn't load the application, and simply `fork()` to create workers. Then, the workers load the application. This is different from when it's not set. Without `--lazy-apps`, master process first load the application then `fork()`. dd-trace-py profiler uses Python stdlib `atexit` module to export the profile when the process exits. The code run to export references global static `std::string` variables. However, when `--lazy-apps` option is set, they are destructed before when the profiler needs them to export the profile, and this leads to an undefined behavior. When `--lazy-apps` is not set the following sequence of events happens 1. uwsgi master process, initialize Python plugin, loads the application. 3. The application calls `import ddtrace.profiling.auto` and this leads all native extensions imported to the Python app. During the process, the constructors for global static variables are called and their destructors would be registered to be called by `atexit()` when the program exits. 4. The application also registers `Profiler.stop()` function to `uwsgi` python module's `atexit` attribute. 5. Then `uwsgi` calls `atexit(uwsgi_plugins_atexit)` to register each plugins atexit callbacks, for Python `uwsgi_python_atexit()` function. And the function will call `Py_Finalize()` 6. Master `fork()`s workers 7. When SIGTERM is received at the master, it sends SIGINT to its workers 8. In each worker, `uwsgi_python_atexit()` is called, invoking `Profiler.stop()` and `Py_Finalize()` is called, then destructors for global static variables are called. When `--lazy-apps` is set 1. uwsgi matser process still intializes Python interpreter, but skips loading the application 3. Then `uwsgi` calls `atexit(uwsgi_plugins_atexit)` to register each plugins atexit callbacks, for Python `uwsgi_python_atexit()` function. And the function will call `Py_Finalize()` 4. Master `fork()`s workers 5. Workers load the application. The application calls `import ddtrace.profiling.auto` and this leads all native extensions imported to the Python app. During the process, the constructors for global static variables are called and their destructors would be registered to be called by C `atexit()` when the program exits. 6. When `--lazy-apps` is set, ddtrace simply uses Python's `atexit` module to call `Profiler.stop()` when the program exits. 7. When SIGTERM is received at the master, it sends SIGINT to its workers 8. In each worker, since global destructors are registered later than `uwsgi_python_atexit()` , they're called first. 9. Then `Py_Finalize()` is called in part of `uwsgi_python_atexit()` calling `Profiler.stop()`. Then it will access those destructed global static strings leading to undefined behavior. To workaround this I have tried three different approaches. 1. #11125: Simply pass the needed `std::string` from Python side to native when exporting pprof file for test 2. #13520: Leak the strings needed for exporting profile to prevent them from destructed 3. #13535: Bundle all needed strings needed for exporting profile, and create them whenever we try to upload. The first approach only addresses issues from tests. The second approach mimics what's suggested in pytorch/pytorch#42125, but since there are a few other global static variables we have throughout the profiler, this still showed crashes from test. And we got similar results from the third approach, as we did from the second. Another way to completely work around this problem is setting `--skip-atexit` as suggested in ansible/ansible-ai-connect-service#248. When `--skip-atexit` is set, `uwsgi` uses `_exit()` instead of `exit()`, and the former doesn't call any functions registered with `atexit()` or `on_exit()`. So, the static variable destructors are never called, and the app no longer crashes. The profiler also doesn't flush the profile via `atexit()` when `--skip-atexit` is set, but I'd argue losing a single profile at the end is better than crashing the application even if the application is exiting. So we'd need to require our customers to set `--skip-atexit` when `--lazy-apps` is set. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
1 parent 95b3d16 commit 22ded2c

File tree

3 files changed

+187
-1
lines changed

3 files changed

+187
-1
lines changed

docs/advanced_usage.rst

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -517,15 +517,21 @@ uWSGI
517517

518518
- Threads must be enabled with the `enable-threads <https://uwsgi-docs.readthedocs.io/en/latest/Options.html#enable-threads>`__ or `threads <https://uwsgi-docs.readthedocs.io/en/latest/Options.html#threads>`__ options.
519519
- Lazy apps must be enabled with the `lazy-apps <https://uwsgi-docs.readthedocs.io/en/latest/Options.html#lazy-apps>`__ option.
520+
- For `uWSGI<2.0.30`, skip atexit, `skip-atexit <https://uwsgi-docs.readthedocs.io/en/latest/Options.html#skip-atexit>`__, must be enabled when `lazy-apps <https://uwsgi-docs.readthedocs.io/en/latest/Options.html#lazy-apps>`__ is enabled. This is to avoid crashes from native extensions that could occur when child processes are terminated.
520521
- For automatic instrumentation (like ``ddtrace-run``) set the `import <https://uwsgi-docs.readthedocs.io/en/latest/Options.html#import>`__ option to ``ddtrace.bootstrap.sitecustomize``.
521522
- Gevent patching should NOT be enabled via `--gevent-patch <https://uwsgi-docs.readthedocs.io/en/latest/Gevent.html#monkey-patching>`__ option. Enabling gevent patching for the builtin threading library is NOT supported. Instead use ``import gevent; gevent.monkey.patch_all(thread=False)`` in your application.
522523

523-
Example with CLI arguments:
524+
Example with CLI arguments for uWSGI>=2.0.30:
524525

525526
.. code-block:: bash
526527
527528
uwsgi --enable-threads --lazy-apps --import=ddtrace.bootstrap.sitecustomize --master --processes=5 --http 127.0.0.1:8000 --module wsgi:app
528529
530+
Example with CLI arguments for uWSGI<2.0.30:
531+
532+
.. code-block:: bash
533+
534+
uwsgi --enable-threads --lazy-apps --skip-atexit --import=ddtrace.bootstrap.sitecustomize --master --processes=5 --http 127.0.0.1:8000 --module wsgi:app
529535
530536
Example with uWSGI ini file:
531537

@@ -542,6 +548,7 @@ Example with uWSGI ini file:
542548
;; ddtrace required options
543549
enable-threads = 1
544550
lazy-apps = 1
551+
skip-atexit = 1 ; For uwsgi<2.0.30
545552
import=ddtrace.bootstrap.sitecustomize
546553
547554
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
other:
3+
- |
4+
docs: Update docs and tests for uwsgi ``--lazy-apps`` config.
5+
For ``uWSGI<2.0.30`` when ``--lazy-apps`` is set , we advise our customers
6+
to also set ``--skip-atexit`` to avoid crashes that could occur from our
7+
native extensions when worker processes are terminated.

tests/profiling_v2/test_uwsgi.py

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
import os
2+
import re
3+
import signal
4+
from subprocess import TimeoutExpired
5+
import sys
6+
import time
7+
8+
import pytest
9+
10+
from tests.contrib.uwsgi import run_uwsgi
11+
from tests.profiling.collector import pprof_utils
12+
13+
14+
# uwsgi is not available on Windows
15+
if sys.platform == "win32":
16+
pytestmark = pytest.mark.skip
17+
18+
TESTING_GEVENT = os.getenv("DD_PROFILE_TEST_GEVENT", False)
19+
THREADS_MSG = (
20+
b"ddtrace.internal.uwsgi.uWSGIConfigError: enable-threads option must be set to true, or a positive "
21+
b"number of threads must be set"
22+
)
23+
24+
uwsgi_app = os.path.join(os.path.dirname(__file__), "..", "profiling", "uwsgi-app.py")
25+
26+
27+
@pytest.fixture
28+
def uwsgi(monkeypatch, tmp_path):
29+
# Do not ignore profiler so we have samples in the output pprof
30+
monkeypatch.setenv("DD_PROFILING_IGNORE_PROFILER", "0")
31+
# Do not use pytest tmpdir fixtures which generate directories longer than allowed for a socket file name
32+
socket_name = str(tmp_path / "uwsgi.sock")
33+
import os
34+
35+
cmd = [
36+
"uwsgi",
37+
"--need-app",
38+
"--die-on-term",
39+
"--socket",
40+
socket_name,
41+
"--wsgi-file",
42+
uwsgi_app,
43+
]
44+
45+
try:
46+
yield run_uwsgi(cmd)
47+
finally:
48+
os.unlink(socket_name)
49+
50+
51+
def test_uwsgi_threads_disabled(uwsgi):
52+
proc = uwsgi()
53+
stdout, _ = proc.communicate()
54+
assert proc.wait() != 0
55+
assert THREADS_MSG in stdout
56+
57+
58+
def test_uwsgi_threads_number_set(uwsgi):
59+
proc = uwsgi("--threads", "1")
60+
try:
61+
stdout, _ = proc.communicate(timeout=1)
62+
except TimeoutExpired:
63+
proc.terminate()
64+
stdout, _ = proc.communicate()
65+
assert THREADS_MSG not in stdout
66+
67+
68+
def test_uwsgi_threads_enabled(uwsgi, tmp_path, monkeypatch):
69+
filename = str(tmp_path / "uwsgi.pprof")
70+
monkeypatch.setenv("DD_PROFILING_OUTPUT_PPROF", filename)
71+
proc = uwsgi("--enable-threads")
72+
worker_pids = _get_worker_pids(proc.stdout, 1)
73+
# Give some time to the process to actually startup
74+
time.sleep(3)
75+
proc.terminate()
76+
assert proc.wait() == 30
77+
for pid in worker_pids:
78+
profile = pprof_utils.parse_profile("%s.%d" % (filename, pid))
79+
samples = pprof_utils.get_samples_with_value_type(profile, "wall-time")
80+
assert len(samples) > 0
81+
82+
83+
def test_uwsgi_threads_processes_no_primary(uwsgi, monkeypatch):
84+
proc = uwsgi("--enable-threads", "--processes", "2")
85+
stdout, _ = proc.communicate()
86+
assert (
87+
b"ddtrace.internal.uwsgi.uWSGIConfigError: master option must be enabled when multiple processes are used"
88+
in stdout
89+
)
90+
91+
92+
def _get_worker_pids(stdout, num_worker, num_app_started=1):
93+
worker_pids = []
94+
started = 0
95+
while True:
96+
line = stdout.readline()
97+
if line == b"":
98+
break
99+
elif b"WSGI app 0 (mountpoint='') ready" in line:
100+
started += 1
101+
else:
102+
m = re.match(r"^spawned uWSGI worker \d+ .*\(pid: (\d+),", line.decode())
103+
if m:
104+
worker_pids.append(int(m.group(1)))
105+
106+
if len(worker_pids) == num_worker and num_app_started == started:
107+
break
108+
109+
return worker_pids
110+
111+
112+
def test_uwsgi_threads_processes_primary(uwsgi, tmp_path, monkeypatch):
113+
filename = str(tmp_path / "uwsgi.pprof")
114+
monkeypatch.setenv("DD_PROFILING_OUTPUT_PPROF", filename)
115+
proc = uwsgi("--enable-threads", "--master", "--py-call-uwsgi-fork-hooks", "--processes", "2")
116+
worker_pids = _get_worker_pids(proc.stdout, 2)
117+
# Give some time to child to actually startup
118+
time.sleep(3)
119+
proc.terminate()
120+
assert proc.wait() == 0
121+
for pid in worker_pids:
122+
profile = pprof_utils.parse_profile("%s.%d" % (filename, pid))
123+
samples = pprof_utils.get_samples_with_value_type(profile, "wall-time")
124+
assert len(samples) > 0
125+
126+
127+
def test_uwsgi_threads_processes_primary_lazy_apps(uwsgi, tmp_path, monkeypatch):
128+
filename = str(tmp_path / "uwsgi.pprof")
129+
monkeypatch.setenv("DD_PROFILING_OUTPUT_PPROF", filename)
130+
monkeypatch.setenv("DD_PROFILING_UPLOAD_INTERVAL", "1")
131+
# For uwsgi<2.0.30, --skip-atexit is required to avoid crashes when
132+
# the child process exits.
133+
proc = uwsgi("--enable-threads", "--master", "--processes", "2", "--lazy-apps", "--skip-atexit")
134+
worker_pids = _get_worker_pids(proc.stdout, 2, 2)
135+
# Give some time to child to actually startup and output a profile
136+
time.sleep(3)
137+
proc.terminate()
138+
assert proc.wait() == 0
139+
for pid in worker_pids:
140+
profile = pprof_utils.parse_profile("%s.%d" % (filename, pid))
141+
samples = pprof_utils.get_samples_with_value_type(profile, "wall-time")
142+
assert len(samples) > 0
143+
144+
145+
def test_uwsgi_threads_processes_no_primary_lazy_apps(uwsgi, tmp_path, monkeypatch):
146+
filename = str(tmp_path / "uwsgi.pprof")
147+
monkeypatch.setenv("DD_PROFILING_OUTPUT_PPROF", filename)
148+
monkeypatch.setenv("DD_PROFILING_UPLOAD_INTERVAL", "1")
149+
# For uwsgi<2.0.30, --skip-atexit is required to avoid crashes when
150+
# the child process exits.
151+
proc = uwsgi("--enable-threads", "--processes", "2", "--lazy-apps", "--skip-atexit")
152+
worker_pids = _get_worker_pids(proc.stdout, 2, 2)
153+
# Give some time to child to actually startup and output a profile
154+
time.sleep(3)
155+
# The processes are started without a master/parent so killing one does not kill the other:
156+
# Kill them all and wait until they die.
157+
for pid in worker_pids:
158+
os.kill(pid, signal.SIGTERM)
159+
# The first worker is our child, we can wait for it "normally"
160+
os.waitpid(worker_pids[0], 0)
161+
# The other ones are grandchildren, we can't wait for it with `waitpid`
162+
for pid in worker_pids[1:]:
163+
# Wait for the uwsgi workers to all die
164+
while True:
165+
try:
166+
os.kill(pid, 0)
167+
except OSError:
168+
break
169+
for pid in worker_pids:
170+
profile = pprof_utils.parse_profile("%s.%d" % (filename, pid))
171+
samples = pprof_utils.get_samples_with_value_type(profile, "wall-time")
172+
assert len(samples) > 0

0 commit comments

Comments
 (0)