promise icon indicating copy to clipboard operation
promise copied to clipboard

Synchronous Promise Resolution Violates Promises/A+ Spec and Causes Zalgo Problem with Event Loop Integration

Open rcalicdan opened this issue 1 month ago • 2 comments

Description

ReactPHP promises execute then() callbacks synchronously when already resolved, violating Promises/A+ spec section 2.2.4 and causing unpredictable execution order with the event loop.

Reproduction

<?php

use React\EventLoop\Loop;
use function React\Promise\resolve;

require 'vendor/autoload.php';

Loop::addTimer(0, function() {
    echo "Timer\n";
});

resolve("promise")->then(function($value) {
    echo $value . "\n";
});

Loop::futureTick(function() {
    echo "future-tick\n";
});

echo "Sync\n";

Actual Output:

promise      ← Executes BEFORE sync code completes!
Sync
future-tick
Timer

Expected Output (Node.js behavior):

Sync
future-tick
promise      ← Should execute asynchronously
Timer

Node.js Comparison

setTimeout(() => console.log("Timer"), 0);
Promise.resolve("promise").then(v => console.log(v));
process.nextTick(() => console.log("next-tick"));
console.log("Sync");

// Output:
// Sync
// next-tick
// promise
// Timer

The Problem

This creates the Zalgo problem - code that is sometimes sync, sometimes async:

function getData($cached) {
    return $cached ? resolve($data) : fetchAsync();
}

$state = 'before';
getData(true)->then(fn() => echo $state);  // Prints 'before' (sync!)
$state = 'after';

$state = 'before';
getData(false)->then(fn() => echo $state); // Prints 'after' (async!)
$state = 'after';

Impact

  • Violates Promises/A+ spec 2.2.4: callbacks must execute asynchronously
  • Unpredictable execution order: depends on promise timing
  • Race conditions: state changes behave differently based on caching
  • Stack overflow risk: recursive promise chains execute synchronously
  • Incompatible with Node.js semantics: make promise timing resolution unpredictable

Root Cause

FulfilledPromise::then() executes callbacks immediately instead of scheduling them:

// src/Internal/FulfilledPromise.php
public function then(?callable $onFulfilled = null, ...): PromiseInterface
{
    try {
        $result = $onFulfilled($this->value); // ← SYNCHRONOUS!
        return resolve($result);
    }
}

Proposed Solution

Integrate with Loop::futureTick() to ensure callbacks always execute asynchronously:

// src/Internal/FulfilledPromise.php
public function then(?callable $onFulfilled = null, ?callable $onRejected = null): PromiseInterface
{
    if (null === $onFulfilled) {
        return $this;
    }

    return new Promise(function($resolve, $reject) use ($onFulfilled) {
        Loop::futureTick(function() use ($onFulfilled, $resolve, $reject) {
            try {
                $result = $onFulfilled($this->value);
                $resolve($result);
            } catch (\Throwable $exception) {
                $reject($exception);
            }
        });
    });
}

Similarly for RejectedPromise::then():

// src/Internal/RejectedPromise.php
public function then(?callable $onFulfilled = null, ?callable $onRejected = null): PromiseInterface
{
    if (null === $onRejected) {
        return $this;
    }

    return new Promise(function($resolve, $reject) use ($onRejected) {
        Loop::futureTick(function() use ($onRejected, $resolve, $reject) {
            try {
                $result = $onRejected($this->reason);
                $resolve($result);
            } catch (\Throwable $exception) {
                $reject($exception);
            }
        });
    });
}

This ensures:

  • Promises/A+ compliance
  • Predictable execution order matching Node.js
  • No Zalgo - always asynchronous
  • Consistent behavior regardless of promise state

Expected Behavior

Promise callbacks should always execute asynchronously via the event loop, regardless of when the promise resolved.

rcalicdan avatar Dec 03 '25 07:12 rcalicdan

Note, that this library advertises to be a Promise/A implementation, not Promise/A+. We had this discussion a couple of times already and also discussed several solutions.

Since PHP has no native event loop, we decided to not couple this library to the ReactPHP event loop so it can be used standalone or with other event loop implementations knowing and accepting the drawback of potential synchronous resolutions.

jsor avatar Dec 03 '25 08:12 jsor

Note, that this library advertises to be a Promise/A implementation, not Promise/A+. We had this discussion a couple of times already and also discussed several solutions.

Since PHP has no native event loop, we decided to not couple this library to the ReactPHP event loop so it can be used standalone or with other event loop implementations knowing and accepting the drawback of potential synchronous resolutions.

This is problematic because it breaks async guarantees and causes many issues, especially race conditions, so in some scenarios especially in React/Async where sometimes an async callback with fiber causes unpredictable timing issues. I think Promises and Event-loop are inseparable to ensure everything works asynchronously. Maybe this team should create a Promise/A+ implementation that is backward compatible with ReactPHP's PromiseInterface to plan for the long term and guarantee async execution and proper task queue semantics.

rcalicdan avatar Dec 03 '25 09:12 rcalicdan

As @jsor mentioned, this library implements Promise/A, not Promise/A+. This is a deliberate design decision, not an oversight. We've discussed this several times over the years.

On standalone usage: PHP has no native event loop, so coupling to react/event-loop would prevent using this library standalone or with alternative event loop implementations. Keeping the promise library decoupled was a conscious choice.

On the Zalgo problem: The example relies on execution order between promise callbacks and mutable state. If your code depends on whether something resolves sync or async, that's a sign you need explicit synchronization. You can't rely on async resolution order between multiple async operations either. If ordering matters, synchronize.

On modern usage: Most code today should use await() from react/async, which provides the synchronization point you're looking for. This makes the sync/async distinction largely irrelevant in practice.

On practical impact: We've had this behavior for years across hundreds of millions of downloads without any real-world issues. Changing it now would be a major BC break, add overhead to every resolution, and introduce a hard dependency, all costs that are hard to justify without concrete problems to solve.

That said, happy to hear more if you have specific use cases where this has caused actual issues. I'm closing this for now as it's more of a design discussion than a bug, but feel free to continue the conversation here.

clue avatar Dec 20 '25 21:12 clue