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

[4.x] Forward compatibility with upcoming Promise v3 #48

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

clue
Copy link
Member

@clue clue commented Jun 12, 2022

Marking this as WIP as the tests currently fail with Promise v3. I've marked the affected tests as incomplete for now, but before merging, the root cause should be fixed instead. Solved via reactphp/promise#229.

Builds on top of reactphp/promise#75 and reactphp/promise#213 and reactphp/promise-timer#54
Changes cherry-picked from #47 for 3.x, but also refs cancellation of async() (see #20 and #42)
Refs #43

@clue clue added new feature New feature or request help wanted Extra attention is needed labels Jun 12, 2022
@clue clue added this to the v4.0.0 milestone Jun 12, 2022
@clue
Copy link
Member 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 suspend fibers when a promise settles which could previously cause some invalid state in internals of promises to break and not execute any other callbacks. 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 this essentially requires suspending the fiber outside of the promise callbacks using a future loop tick. This would essentially revert the major improvements we've recently added via #32 and would thus cause regressions for #27.

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

@clue clue changed the title [WIP] [4.x] Forward compatibility with upcoming Promise v3 [4.x] Forward compatibility with upcoming Promise v3 Jun 26, 2022
@clue
Copy link
Member Author

clue commented Jun 26, 2022

Updated PR now that reactphp/promise#229 has been merged, this is now ready to be shipped :shipit:

@clue
Copy link
Member Author

clue commented Jun 30, 2022

Rebased to resolve merge conflict now that #55 is in. This also happens to remove the dependency on PromiseTimer, so this no longer refs reactphp/promise-timer#54 :shipit:

@WyriHaximus WyriHaximus self-requested a review June 30, 2022 09:14
@WyriHaximus WyriHaximus merged commit 257634a into reactphp:4.x Jun 30, 2022
@clue clue deleted the promise-v4 branch June 30, 2022 09:22
@clue clue mentioned this pull request Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants