PR comments

Author: cataphract

Diff for: ext/integrations/exec_integration.c

@@ -21,7 +21,7 @@ typedef struct php_process_handle php_process_handle;
 /* popen stream handler close interception */
 
 static int dd_php_stdiop_close_wrapper(php_stream *stream, int close_handle);
-static bool dd_waitpid(ddtrace_span_data *, php_process_id_t);
+static void dd_waitpid(ddtrace_span_data *, php_process_id_t);
 
 ZEND_TLS HashTable *tracked_streams;  // php_stream => span
 static zend_string *cmd_exit_code_zstr;
@@ -110,15 +110,15 @@ static int le_proc;
 static int le_proc_span;
 
 static void dd_proc_wrapper_rsrc_dtor(zend_resource *rsrc) {
-    // this is called from the begginning of proc_open_rsrc_dtor,
+    // this is called from the beginning of proc_open_rsrc_dtor,
     // before the process is possibly reaped
 
     dd_proc_span *proc_span = (dd_proc_span *)rsrc->ptr;
 
     ddtrace_span_data *span_data = OBJ_SPANDATA(proc_span->span);
 
     if (span_data->duration == 0) {
-        (void)dd_waitpid(span_data, proc_span->child);
+        dd_waitpid(span_data, proc_span->child);
 
         dd_trace_stop_span_time(span_data);
         ddtrace_close_span_restore_stack(span_data);
@@ -128,10 +128,10 @@ static void dd_proc_wrapper_rsrc_dtor(zend_resource *rsrc) {
     efree(proc_span);
 }
 
-static bool /* reaped */ dd_waitpid(ddtrace_span_data *span_data, php_process_id_t pid) {
+static void dd_waitpid(ddtrace_span_data *span_data, php_process_id_t pid) {
     if (span_data->duration) {
         // already closed
-        return false;
+        return;
     }
     zend_array *meta = ddtrace_property_array(&span_data->property_meta);
 
@@ -146,10 +146,13 @@ static bool /* reaped */ dd_waitpid(ddtrace_span_data *span_data, php_process_id
     }
 
     if (pid_res != pid) {
-        return false;  // some other error. Probably the process is no more/not a child
+        return;  // 0 was returned (no changed status) or some error
+                 // Probably the process is no more/not a child
     }
 
+    bool exited = false;
     if (WIFEXITED(wstatus)) {
+        exited = true;
         wstatus = WEXITSTATUS(wstatus);
     } else if (WIFSIGNALED(wstatus)) {
         // wstatus is not modified!
@@ -160,18 +163,16 @@ static bool /* reaped */ dd_waitpid(ddtrace_span_data *span_data, php_process_id
         zval has_signalled_zv;
         ZVAL_INTERNED_STR(&has_signalled_zv, has_signalled_zstr);
         zend_hash_update(meta, error_message_zstr, &has_signalled_zv);
+        exited = true;
     }  // else !FG(pclose_wait) and it hasn't finished
 
-    if (wstatus != -1) {  // we matched one of the two branches above
+    if (exited) {
         zval zexit;
         ZVAL_LONG(&zexit, wstatus);
 
         // set tag 'cmd.exit_code'
         zend_hash_update(meta, cmd_exit_code_zstr, &zexit);
-        return true;
     }
-
-    return false;
 }
 
 static PHP_FUNCTION(DDTrace_integrations_exec_proc_assoc_span) {

Diff for: src/Integrations/Integrations/Exec/ExecIntegration.php

@@ -77,6 +77,17 @@ static function (HookData $hook) {
         );
 
 
+        /*
+         * This instrumentation works by creating a span on the enter callback, and then
+         * associating this span with the resource returned by proc_open. This association
+         * is done by adding a resource to the list of pipes of the proc resource. This
+         * resource (of type dd_proc_span) is not an actual pipe, but it doesn't matter;
+         * PHP will only ever destroy this resource.
+         *
+         * When the proc resource is destroyed, the dd_proc_span resource is destroyed as
+         * well, and in the process the span is finished, unless it was finished before
+         * in proc_get_status.
+         */
         \DDTrace\install_hook(
             'proc_open',
             static function (HookData $hook) {