Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward compatibility with upcoming Promise v3 #157

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

clue
Copy link
Contributor

@clue clue commented Jun 22, 2022

@clue
Copy link
Contributor Author

clue commented Jun 23, 2022

I've updated this to show that this is currently only blocked by reactphp/promise#229. The gist is that we resolve a number of promises in the same tick and its internal queue mechanism would use a different execution order for Promise v3 only. This is now addressed upstream for Promise v3 as this is the only version that shows this problem.

As an alternative, I've also looked into addressing this in this component instead, but I believe the logic to be fine and the previous Promise v3 behavior to be counter-intuitive. The code in question looks good to me and suggests outstanding promises should be rejected before the final close event is emitted:

mysql/src/Io/Connection.php

Lines 163 to 181 in 7b4428d

// reject all pending commands if connection is closed
while (!$this->executor->isIdle()) {
$command = $this->executor->dequeue();
assert($command instanceof CommandInterface);
if ($remoteClosed) {
$command->emit('error', [new \RuntimeException(
'Connection closed by peer (ECONNRESET)',
\defined('SOCKET_ECONNRESET') ? \SOCKET_ECONNRESET : 104
)]);
} else {
$command->emit('error', [new \RuntimeException(
'Connection closing (ECONNABORTED)',
\defined('SOCKET_ECONNABORTED') ? \SOCKET_ECONNABORTED : 103
)]);
}
}
$this->emit('close');

As such, I believe addressing this in reactphp/promise#229 is the best way moving forward. The resulting patch looks surprisingly straight-forward, but together with the referenced tickets you're looking at several days worth of work on investigating this root cause and coming up with this minimal solution. Enjoy!

@clue
Copy link
Contributor Author

clue commented Jun 26, 2022

Updated now that reactphp/promise#229 has been merged, this now only depends on reactphp/socket#214.

The first commit updates this to the currently unreleased Socket component to show how this only depends on reactphp/socket#214 (the build should be green). The second commit updates this to the releases that have yet to be tagged. This is expected to fail at the moment and should be green once the releases are tagged and the build is restarted.

@clue clue changed the title [WIP] Forward compatibility with upcoming Promise v3 Forward compatibility with upcoming Promise v3 Aug 25, 2022
@clue
Copy link
Contributor Author

clue commented Aug 25, 2022

Restarted build now that Socket v1.12.0 has been released, this is now ready for review :shipit:

@clue clue requested a review from WyriHaximus August 30, 2022 18:07
@WyriHaximus WyriHaximus merged commit 81d8362 into friends-of-reactphp:0.5.x Aug 30, 2022
@clue clue deleted the promise-v3 branch August 30, 2022 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants