friends-of-reactphp / mysql

Async MySQL database client for ReactPHP.
MIT License
331 stars 66 forks source link

Forward compatibility with upcoming Promise v3 #157

Closed clue closed 1 year ago

clue commented 2 years ago

Builds on top of https://github.com/reactphp/promise/pull/213, https://github.com/reactphp/promise/pull/138, https://github.com/reactphp/promise/pull/224, https://github.com/reactphp/socket/pull/214 and https://github.com/reactphp/promise-stream/pull/20, https://github.com/reactphp/promise-timer/pull/54 and https://github.com/clue/reactphp-block/pull/61 Refs https://github.com/clue/reactphp-http-proxy/pull/44

clue commented 2 years ago

I've updated this to show that this is currently only blocked by https://github.com/reactphp/promise/pull/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: https://github.com/friends-of-reactphp/mysql/blob/7b4428da4d625787a0ce1420b497dadcd70ff7bd/src/Io/Connection.php#L163-L181

As such, I believe addressing this in https://github.com/reactphp/promise/pull/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 commented 2 years ago

Updated now that https://github.com/reactphp/promise/pull/229 has been merged, this now only depends on https://github.com/reactphp/socket/pull/214.

The first commit updates this to the currently unreleased Socket component to show how this only depends on https://github.com/reactphp/socket/pull/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 commented 1 year ago

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