Test failure with 8.3.0RC1

[remicollet]

TEST 6/29 [tests/eio_custom_basic.phpt]
========DIFF========
     resource(%d) of type (EIO Request Descriptor)
002- string(14) "my_custom_data"
003- string(14) "my_custom_data"
004- int(2)
005- string(14) "my custom data"
006- int(1001)
002+ 
003+ Fatal error: Uncaught Error: Maximum call stack size of 8339456 bytes reached. Infinite recursion? in /dev/shm/BUILD/php83-php-pecl-eio-3.1.0~RC1/eio-3.1.0RC1/tests/eio_custom_basic.php:12
004+ Stack trace:
005+ #0 [internal function]: my_custom('my_custom_data')
006+ #1 /dev/shm/BUILD/php83-php-pecl-eio-3.1.0~RC1/eio-3.1.0RC1/tests/eio_custom_basic.php(26): eio_event_loop()
007+ #2 {main}
008+   thrown in /dev/shm/BUILD/php83-php-pecl-eio-3.1.0~RC1/eio-3.1.0RC1/tests/eio_custom_basic.php on line 12
========DONE========
FAIL Check for eio_custom function basic behaviour [tests/eio_custom_basic.phpt] 

[rosmanov]

@remicollet, I noticed the following note in the UPGRADING.INTERNALS file:

• SAPIs that may execute in alternative stacks have to set EG(stack_limit) and
  EG(stack_base)

Since libeio uses a custom thread to execute the user callback passed to eio_custom, it appears necessary to initialize the stack here. But I'm unsure of the precise steps to do so.

I've successfully resolved the issue with the following patch:

diff --git a/php8/php_eio.c b/php8/php_eio.c
index e79d10f..c87ca82 100644
--- a/php8/php_eio.c
+++ b/php8/php_eio.c
@@ -479,6 +479,10 @@ static void php_eio_custom_execute(eio_req *req)
 			ZVAL_NULL(&zarg);
 		}
 
+#ifdef ZEND_CHECK_STACK_LIMIT
+		zend_call_stack_init();
+#endif
+
 		zend_call_method(Z_ISUNDEF(pf->obj) ? NULL : Z_OBJ_P(&pf->obj), pf->ce, &pf->func_ptr,
 				ZSTR_VAL(pf->func_ptr->common.function_name),
 				ZSTR_LEN(pf->func_ptr->common.function_name),
@@ -863,8 +867,11 @@ static inline void php_eio_init()
 	pid_t cur_pid = getpid();
 
 	if (php_eio_pid <= 0 || (php_eio_pid > 0 && cur_pid != php_eio_pid)) {
-		/* Uninitialized or forked a process(which needs it's own eio pipe) */
+#ifdef ZEND_CHECK_STACK_LIMIT
+		zend_call_stack_init();
+#endif
 
+		/* Uninitialized or forked a process(which needs it's own eio pipe) */
 		if (php_eio_pipe_new()) {
 			php_error_docref(NULL, E_ERROR,
 					"Failed creating internal pipe: %s", strerror(errno));

The patch invokes zend_call_stack_init() in two places:

1. Within the php_eio_custom_execute function, which runs in a custom thread.
2. In the php_eio_init function, triggered during a fork: X_THREAD_ATFORK(NULL, NULL, php_eio_atfork_child);

I'm particularly uncertain about the first instance. You thoughts on this?

[rosmanov]

There is also EIO_STACKSIZE compile-time option in the config.m4. Do you think I should tweak it as well?

[remicollet]

Perhaps @arnaud-lb may help on this ? (as the author of the PHP change)

[arnaud-lb]

This is the right approach 👍 Threads have their own stack, so EG(stack_limit) and EG(stack_base) must be updated before executing PHP code in the thread. Calling zend_call_stack_init() is the right way to do it, unless you already know where the stack is, in which case you can directly assign EG(stack_limit) and EG(stack_base) to the lowest and highest addresses of the stack, respectively. (Subtract EG(reserved_stack_size) from the highest one.)

You also need to restore the previous value of EG(stack_limit) and EG(stack_base) when switching back to the main thread.

It's probably worth it to cache the value of EG(stack_limit) and EG(stack_base) after calling zend_call_stack_init() if you switch to the thread more than once.

[remicollet]

@rosmanov testing 3.1.0RC1 with your patch, build OK and test suite passes

[rosmanov]

This is the right approach 👍 Threads have their own stack, so EG(stack_limit) and EG(stack_base) must be updated before executing PHP code in the thread. Calling zend_call_stack_init() is the right way to do it, unless you already know where the stack is, in which case you can directly assign EG(stack_limit) and EG(stack_base) to the lowest and highest addresses of the stack, respectively. (Subtract EG(reserved_stack_size) from the highest one.)

You also need to restore the previous value of EG(stack_limit) and EG(stack_base) when switching back to the main thread.

It's probably worth it to cache the value of EG(stack_limit) and EG(stack_base) after calling zend_call_stack_init() if you switch to the thread more than once.

@arnaud-lb, thank you for your answer! It's nice to hear that I'm on the right track. However, I'm still unsure about a couple of points:

1. This extension doesn't know when threads are created (by the libeio library it wraps). The only way for me to fix the stack is to call zend_call_stack_init() from within a callback invoked by libeio in a thread function. That seems to work, but I'm not completely sure if this is the right way.

2. The PHP code is executed from a function of a thread created in detached mode (pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);). Therefore, I think I don't need to save and restore the EG(stack_limit) and EG(stack_base) values. Is that correct?

3. I'm also a little bit concerned about a possible performance penalty due to the zend_call_stack_init() call, although the function appears to be lightweight enough.

[arnaud-lb]

1. This extension doesn't know when threads are created (by the libeio library it wraps). The only way for me to fix the stack is to call zend_call_stack_init() from within a callback invoked by libeio in a thread function. That seems to work, but I'm not completely sure if this is the right way.

It's the right way then. You just need to restore the original stack addresses after executing the callback:

diff --git a/php8/php_eio.c b/php8/php_eio.c
index e79d10f..2d5bed5 100644
--- a/php8/php_eio.c
+++ b/php8/php_eio.c
@@ -473,6 +473,10 @@ static void php_eio_custom_execute(eio_req *req)
 
        pf = &eio_cb->func_exec;
        if (EXPECTED(pf->func_ptr)) {
+               void *stack_limit = EG(stack_limit);
+               void *stack_base = EG(stack_base);
+               zend_call_stack_init();
+
                if (! Z_ISUNDEF(eio_cb->arg)) {
                        ZVAL_COPY(&zarg, &eio_cb->arg);
                } else {
@@ -494,6 +498,9 @@ static void php_eio_custom_execute(eio_req *req)
                zend_exception_restore();
 
                zval_ptr_dtor(&zarg);
+
+               EG(stack_limit) = stack_limit;
+               EG(stack_base) = stack_base;
        }
 #endif /* ZTS */
 }

If I understand correctly, the main thread never executes PHP code during eio_event_loop(). In that case, you probably don't need to restore the stack addresses after each callback as long as you do it before returning from eio_event_loop().

2. The PHP code is executed from a function of a thread created in detached mode (pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);). Therefore, I think I don't need to save and restore the EG(stack_limit) and EG(stack_base) values. Is that correct?

I'm not sure. If PHP code can be executed in the main thread after that, you need to restore the stack addresses.

3. I'm also a little bit concerned about a possible performance penalty due to the zend_call_stack_init() call, although the function appears to be lightweight enough.

If that's an issue, maybe you can cache the stack addresses in thread-specific data. I wouldn't attempt that before benchmarking though.

[arnaud-lb]

NB: Beware of zval_ptr_dtor(), since it can execute destructors. You need to move the zend_call_stack_init(); before php_eio_free_eio_cb_custom() in php_eio_custom_execute().

[rosmanov]

@remicollet, I hope that fixes the issue. I've uploaded a new PECL release version 3.0.1.

Thank you guys.

[remicollet]

Thanks for 3.0.1

Do you have any plans to release 3.1.0 soon?

RC1 is 3 months old, and as there is no previous stable version of 3.x, 3.1.0RC1 was the version available to RPM users
Updating to 3.0.1 doesn't seem possible easily (looks like a downgrade)

[remicollet]

Additional notice:

In source code, version was 3.0.0RC5, but it was released on pecl as 3.1.0RC1 (no tag here)
See https://pecl.php.net/package-changelog.php?package=eio

[rosmanov]

Thanks for 3.0.1

Do you have any plans to release 3.1.0 soon?

RC1 is 3 months old, and as there is no previous stable version of 3.x, 3.1.0RC1 was the version available to RPM users Updating to 3.0.1 doesn't seem possible easily (looks like a downgrade)

Do you think I could just bump version to 3.1.0 (stable)?

[rosmanov]

Uploaded eio-3.1.0.

Thanks