Skip to content

Commit

Permalink
chore(gevent): run after-in-child hooks after reinit (#4070)
Browse files Browse the repository at this point in the history
## Description

Threads created too early in an application that uses gevent end up not running when the gevent hub reinit is executed after fork in the child process. This change ensures that we trigger the after-in-child fork hooks after the call to `gevent.hub.reinit` to ensure that threads created at any time will run as expected after fork.

### More details

In its implementation, `gevent` wraps around `os.fork` and calls `gevent.hub.reinit` to re-initialise the state of the child process after fork. The `forksafe` hooks that are registered by the library end up running after the call to the OS `fork` syscall, but *before* `gevent.hub.reinit`, which causes the threads to not work as expected in the child process. Other frameworks, like `gunicorn` make their own call to `os.fork` and then call `gevent.hub.reinit` in the child process to obtain the same effect. This poses the same problem as in plain `gevent`. With this change, we register an import hook on `gevent.hub` to detect the possibility that `gevent.hub.reinit` might be called, and we patch this function to re-run the after-in-child fork hook to ensure that they run *after* the gevent hub has been reinitialised fully in the child process after fork. In particular, this ensures that threads are recreated at the right time and can then work as expected in child processes.

## Checklist
- [x] Title must conform to [conventional commit](https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional).
- [x] Add additional sections for `feat` and `fix` pull requests.
- [x] Ensure tests are passing for affected code.
- [x] [Library documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs) and/or [Datadog's documentation site](https://github.com/DataDog/documentation/) is updated. Link to doc PR in description.

## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] PR cannot be broken up into smaller PRs.
- [ ] Avoid breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is included in the code or PR.
- [ ] Release note has been added for fixes and features, or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.
- [ ] Add to milestone.
  • Loading branch information
P403n1x87 authored Aug 19, 2022
1 parent 7f0943a commit 9b05c3b
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 0 deletions.
20 changes: 20 additions & 0 deletions ddtrace/internal/forksafe.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import typing
import weakref

from ddtrace.internal.module import ModuleWatchdog
from ddtrace.vendor import wrapt


Expand All @@ -22,6 +23,25 @@
_soft = False


def patch_gevent_hub_reinit(module):
# The gevent hub is re-initialized *after* the after-in-child fork hooks are
# called, so we patch the gevent.hub.reinit function to ensure that the
# fork hooks run again after this further re-initialisation, if it is ever
# called.
from ddtrace.internal.wrapping import wrap

def wrapped_reinit(f, args, kwargs):
try:
return f(*args, **kwargs)
finally:
ddtrace_after_in_child()

wrap(module.reinit, wrapped_reinit)


ModuleWatchdog.register_module_hook("gevent.hub", patch_gevent_hub_reinit)


def ddtrace_after_in_child():
# type: () -> None
global _registry
Expand Down
1 change: 1 addition & 0 deletions riotfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ def select_pys(min_version=MIN_PYTHON_VERSION, max_version=MAX_PYTHON_VERSION):
"structlog": latest,
# httpretty v1.0 drops python 2.7 support
"httpretty": "==0.9.7",
"gevent": latest,
},
)
],
Expand Down
57 changes: 57 additions & 0 deletions tests/tracer/test_forksafe.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import sys

import pytest
import six
Expand Down Expand Up @@ -285,3 +286,59 @@ def fn():
pid, status = os.waitpid(child, 0)
exit_code = os.WEXITSTATUS(status)
assert exit_code == 42


@pytest.mark.subprocess(
out="" if (3,) < sys.version_info < (3, 7) else ("CTCTCT" if sys.platform == "darwin" else "CCCTTT"),
err=None,
)
def test_gevent_reinit_patch():
import os
import sys

from ddtrace.internal import forksafe
from ddtrace.internal.periodic import PeriodicService

class TestService(PeriodicService):
def __init__(self):
super(TestService, self).__init__(interval=1.0)

def periodic(self):
sys.stdout.write("T")

service = TestService()
service.start()

def restart_service():
global service

service.stop()
service = TestService()
service.start()

forksafe.register(restart_service)

import gevent # noqa

def run_child():
global service

# We mimic what gunicorn does in child processes
gevent.monkey.patch_all()
gevent.hub.reinit()

sys.stdout.write("C")

gevent.sleep(1.5)

service.stop()

def fork_workers(num):
for _ in range(num):
if os.fork() == 0:
run_child()
sys.exit(0)

fork_workers(3)

service.stop()

0 comments on commit 9b05c3b

Please sign in to comment.