Fix exception raised by php_error_docref that hangs the process in …

…hook (#127) * Use E_CORE_WARNING for logging * Add test, fix other tests --------- Co-authored-by: Brett McBride <[email protected]>
open-telemetry · Jan 27, 2024 · dec47b1 · dec47b1
1 parent a113718
commit dec47b1
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 9 deletions.
diff --git a/.gitignore b/.gitignore
@@ -1,3 +1,4 @@
 .idea
+.vscode/
 *.tgz
 
diff --git a/ext/otel_observer.c b/ext/otel_observer.c
@@ -303,7 +303,7 @@ static void exception_isolation_handle_exception(zend_object *suppressed,
         zend_read_property_ex(exception_base, suppressed,
                               ZSTR_KNOWN(ZEND_STR_MESSAGE), 1, &return_value);
 
-    php_error_docref(NULL, E_WARNING,
+    php_error_docref(NULL, E_CORE_WARNING,
                      "OpenTelemetry: %s threw exception,"
                      " class=%s function=%s message=%s",
                      type, zval_get_chars(class_name),
@@ -376,7 +376,7 @@ static void observer_begin(zend_execute_data *execute_data, zend_llist *hooks) {
         fci.retval = &ret;
 
         if (!is_valid_signature(fci, fcc)) {
-            php_error_docref(NULL, E_WARNING,
+            php_error_docref(NULL, E_CORE_WARNING,
                              "OpenTelemetry: pre hook invalid signature,"
                              " class=%s function=%s",
                              (Z_TYPE_P(&params[2]) == IS_NULL)
@@ -407,7 +407,7 @@ static void observer_begin(zend_execute_data *execute_data, zend_llist *hooks) {
 
                         if (idx == (uint32_t)-1) {
                             php_error_docref(
-                                NULL, E_NOTICE,
+                                NULL, E_CORE_WARNING,
                                 "OpenTelemetry: pre hook unknown "
                                 "named arg %s, class=%s function=%s",
                                 ZSTR_VAL(str_idx), zval_get_chars(&params[2]),
@@ -423,7 +423,7 @@ static void observer_begin(zend_execute_data *execute_data, zend_llist *hooks) {
                             continue;
                         }
 
-                        php_error_docref(NULL, E_NOTICE,
+                        php_error_docref(NULL, E_CORE_WARNING,
                                          "OpenTelemetry: pre hook invalid "
                                          "argument index " ZEND_ULONG_FMT
                                          ", class=%s function=%s",
@@ -543,7 +543,7 @@ static void observer_end(zend_execute_data *execute_data, zval *retval,
         fci.retval = &ret;
 
         if (!is_valid_signature(fci, fcc)) {
-            php_error_docref(NULL, E_WARNING,
+            php_error_docref(NULL, E_CORE_WARNING,
                              "OpenTelemetry: post hook invalid signature, "
                              "class=%s function=%s",
                              (Z_TYPE_P(&params[4]) == IS_NULL)

diff --git a/ext/package.xml b/ext/package.xml
@@ -77,6 +77,7 @@
         <file name="function_internal.phpt" role="test"/>
         <file name="hooks_throw_exception.phpt" role="test"/>
         <file name="hooks_throw_exception_for_failed_call.phpt" role="test"/>
+        <file name="hooks_throw_exception_error_handler.phpt" role="test"/>
         <file name="ini_set.phpt" role="test"/>
         <file name="invalid_post_callback_signature.phpt" role="test"/>
         <file name="invalid_pre_callback_signature.phpt" role="test"/>

diff --git a/ext/tests/expand_params_extra.phpt b/ext/tests/expand_params_extra.phpt
@@ -23,7 +23,7 @@ function helloWorld($a, $b) {
 helloWorld('a');
 ?>
 --EXPECTF--
-Notice: helloWorld(): OpenTelemetry: pre hook invalid argument index 2, class=null function=helloWorld in %s
+Warning: helloWorld(): OpenTelemetry: pre hook invalid argument index 2, class=null function=helloWorld in %s
 array(2) {
   [0]=>
   string(1) "a"

diff --git a/ext/tests/expand_params_internal.phpt b/ext/tests/expand_params_internal.phpt
@@ -23,7 +23,7 @@ OpenTelemetry\Instrumentation\hook(
 var_dump(array_slice([1,2,3], 1));
 ?>
 --EXPECTF--
-Notice: array_slice(): OpenTelemetry: pre hook invalid argument index 2, class=null function=array_slice in %s
+Warning: array_slice(): OpenTelemetry: pre hook invalid argument index 2, class=null function=array_slice in %s
 array(2) {
   [0]=>
   int(2)

diff --git a/ext/tests/hooks_throw_exception_error_handler.phpt b/ext/tests/hooks_throw_exception_error_handler.phpt
@@ -0,0 +1,32 @@
+--TEST--
+Check if exceptions thrown in hooks work if custom error handler throws
+--DESCRIPTION--
+If the extension internally logs errors/warnings in a way that set_error_handler gets invoked, then any
+exceptions/errors may cause the process to crash or hang if raising a throwable was not safe at that moment.
+--EXTENSIONS--
+opentelemetry
+--FILE--
+<?php
+set_error_handler(function (int $errno, string $message) {
+    throw new \Error('Throw from error handler: ' . $message);
+});
+function helloWorld() {
+    throw new \Error('test');
+}
+\OpenTelemetry\Instrumentation\hook(
+    null,
+    'helloWorld',
+    pre: static function () : void {
+        throw new \Exception('pre');
+    }
+);
+helloWorld();
+?>
+--EXPECTF--
+Warning: helloWorld(): OpenTelemetry: pre hook threw exception, class=null function=helloWorld message=pre in %s
+
+Fatal error: Uncaught Error: test in %s
+Stack trace:
+#0 %s: helloWorld()
+#1 {main}
+  thrown in %s
diff --git a/ext/tests/modify_named_params_invalid.phpt b/ext/tests/modify_named_params_invalid.phpt
@@ -20,7 +20,7 @@ function hello($one = null, $two = null, $three = null) {
 hello('a', 'b', 'c');
 ?>
 --EXPECTF--
-Notice: hello(): OpenTelemetry: pre hook unknown named arg four, class=null function=hello in %s
+Warning: hello(): OpenTelemetry: pre hook unknown named arg four, class=null function=hello in %s
 array(3) {
   [0]=>
   string(1) "a"

diff --git a/ext/tests/return_expanded_params_internal.phpt b/ext/tests/return_expanded_params_internal.phpt
@@ -21,7 +21,7 @@ opentelemetry
 var_dump(array_slice(['a', 'b', 'c'], 1));
 ?>
 --EXPECTF--
-Notice: array_slice(): OpenTelemetry: pre hook invalid argument index 2, class=null function=array_slice in %s
+Warning: array_slice(): OpenTelemetry: pre hook invalid argument index 2, class=null function=array_slice in %s
 array(2) {
   [0]=>
   string(1) "b"