Skip to content

Commit

Permalink
Add missing "in-preloading" check to RSHUTDOWN
Browse files Browse the repository at this point in the history
`RSHUTDOWN` was missing the "in-preloading" check which would lead to a
situation where some profilers would collect data even though in
preloading. In allocation profiling this would lead to a situation where
if a customer has a custom memory handler (or uses `USE_ZEND_ALLOC=0`)
the allocation profiler would crash in
`allocation::alloc_prof_rshutdown()` when trying to reset the heap.
  • Loading branch information
realFlowControl committed Jan 14, 2025
1 parent c73b7f7 commit 1000e1c
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 20 deletions.
40 changes: 23 additions & 17 deletions profiling/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,23 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
locals.system_settings = system_settings;
});

// Preloading happens before zend_post_startup_cb is called for the first
// time. When preloading is enabled and a non-root user is used for
// php-fpm, there is fork that happens. In the past, having the profiler
// enabled at this time would cause php-fpm eventually hang once the
// Profiler's channels were full; this has been fixed. See:
// https://github.com/DataDog/dd-trace-php/issues/1919
//
// There are a few ways to handle this preloading scenario with the fork,
// but the simplest is to not enable the profiler until the engine's
// startup is complete. This means the preloading will not be profiled,
// but this should be okay.
#[cfg(php_preload)]
if !unsafe { bindings::ddog_php_prof_is_post_startup() } {
debug!("zend_post_startup_cb hasn't happened yet; not enabling profiler.");
return ZendResult::Success;
}

// SAFETY: still safe to access in rinit after first_rinit.
let system_settings = unsafe { system_settings.as_mut() };

Expand Down Expand Up @@ -531,23 +548,6 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
exception::exception_profiling_first_rinit();
});

// Preloading happens before zend_post_startup_cb is called for the first
// time. When preloading is enabled and a non-root user is used for
// php-fpm, there is fork that happens. In the past, having the profiler
// enabled at this time would cause php-fpm eventually hang once the
// Profiler's channels were full; this has been fixed. See:
// https://github.com/DataDog/dd-trace-php/issues/1919
//
// There are a few ways to handle this preloading scenario with the fork,
// but the simplest is to not enable the profiler until the engine's
// startup is complete. This means the preloading will not be profiled,
// but this should be okay.
#[cfg(php_preload)]
if !unsafe { bindings::ddog_php_prof_is_post_startup() } {
debug!("zend_post_startup_cb hasn't happened yet; not enabling profiler.");
return ZendResult::Success;
}

Profiler::init(system_settings);

if system_settings.profiling_enabled {
Expand Down Expand Up @@ -631,6 +631,12 @@ extern "C" fn rshutdown(_type: c_int, _module_number: c_int) -> ZendResult {
#[cfg(debug_assertions)]
trace!("RSHUTDOWN({_type}, {_module_number})");

#[cfg(php_preload)]
if !unsafe { bindings::ddog_php_prof_is_post_startup() } {
debug!("zend_post_startup_cb hasn't happened yet; not disabling profiler.");
return ZendResult::Success;
}

profiling::stack_walking::rshutdown();

REQUEST_LOCALS.with(|cell| {
Expand Down
7 changes: 4 additions & 3 deletions profiling/tests/phpt/preload_01.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,18 @@ See: https://github.com/DataDog/dd-trace-php/issues/1919
Due to limitations of PHPT, this test does not test PHP-FPM or processes run
with different permissions, but it makes sure, that profiling is started after
preloading is done.
--ENV--
USE_ZEND_ALLOC=0
--SKIPIF--
<?php
if (PHP_VERSION_ID < 70400)
echo "skip: need preloading and therefore PHP", PHP_EOL;
echo "skip: need preloading and therefore PHP >= 7.4.0", PHP_EOL;
if (!extension_loaded('datadog-profiling'))
echo "skip: test requires datadog-profiling", PHP_EOL;
?>
--INI--
datadog.profiling.enabled=yes
datadog.profiling.log_level=debug
datadog.profiling.allocation_enabled=no
datadog.profiling.experimental_cpu_time_enabled=no
zend_extension=opcache
opcache.enable_cli=1
opcache.preload={PWD}/preload_01_preload.php
Expand All @@ -41,5 +41,6 @@ echo "Done.", PHP_EOL;
--EXPECTREGEX--
.*zend_post_startup_cb hasn't happened yet; not enabling profiler.
preloading
.*zend_post_startup_cb hasn't happened yet; not disabling profiler.
.*Started with an upload period of [0-9]+ seconds and approximate wall-time period of [0-9]+ milliseconds.
.*Done.*

0 comments on commit 1000e1c

Please sign in to comment.