-
Notifications
You must be signed in to change notification settings - Fork 22
Add async/promise support to Twirp PHP clients #225
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
base: master
Are you sure you want to change the base?
Conversation
This commit introduces asynchronous request processing for Twirp PHP clients using Guzzle's promise framework that adheres to PSR-7 standards. Key features: - All RPC methods now have async variants (e.g., MethodNameAsync()) - Returns promises for non-blocking HTTP requests - Supports concurrent requests using Guzzle promises - Automatic fallback to synchronous execution for non-Guzzle clients - Full backward compatibility with existing synchronous methods - Both Protobuf and JSON clients support async operations Implementation details: - AbstractClient: Added async method generation and doRequestAsync() - Client/JsonClient: Implemented async request handling with promises - Service Interface: Added async method signatures - Documentation: Added comprehensive async-processing.rst in docs/advanced/ - Updated test implementations to support async methods All existing tests pass, ensuring backward compatibility.
|
Hey! Thanks a lot for this PR! I really appreciate the high quality! I'm at a conference right now, but I'll review it as soon as I can. |
|
@sagikazarmark just following up if there is any feedback I can work on. |
SpencerMalone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently guzzle is a dev requirement, I think as part of this PR we would also need to promote it to a prod requirement, right?
| return $hat; | ||
| } | ||
|
|
||
| public function MakeHatAsync(array $ctx, Size $size): PromiseInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, we'd have to add this on all server implementations since Server.php.tmpl takes in a _Service_.php.tmpl in the constructor, which I think is a breaking change. What do you think if we provided a trait and/or abstract class (honestly maybe both? a trait would be nice for people who already have an existing dependency chain and can't use the abstract class) that provides this default promise behavior so that people can just use that new trait and not have to make a bunch of these functions?
Actually, now that I think more: Is it even valuable to have these async methods on the service interfaces? I think this might be adding a bunch of friction to this feature for something that will be incredibly niche.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if adding a separate interface would help. Clients could always implement that interface, but servers wouldn't have to.
I agree, forcing service implementations to add this seems unnecessary and creates friction.
Thoughts @winmutt ?
sagikazarmark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @winmutt
It's really great work. I rarely see this quality in open source contributions, so kudos.
I fully support getting async into the library.
I'd like to raise a few thoughts though before we move forward with this PR.
First, I agree with @SpencerMalone that adding the Async methods to the interface may not be necessary. At least I don't see the added benefit of being able to write async handlers on the server side. The difference is basically who resolves the promise: the framework or the user (IF there is any async code inside the handler).
Am I wrong?
As an alternative, I suggested creating a separate interface for clients to implement. You can still code against that in your client consumers. Technically, that breaks the RPC abstraction, but given PHP has no language constructs for async code, I think we can live with that.
Another thing I wanted to raise is the Guzzle dependency. I wonder if we could get away with something simpler that allows future expansion.
For example:
- I'm not sure about the adoption of HTTPlug async these days (which is funny, because I wrote that library 😄 ), but it comes with a lightweight promise abstraction too and provides a guzzle adapter.
- Alternatively, maybe we could deliver a simple Promise interface with the library. (Although that would make interoperability with other libraries harder)
To be clear: I'm not sure having Guzzle as a dependency is wrong. This may just be my inner overengineer speaking. Nevertheless, I wanted to raise this as a discussion.
I'd hate to see people not using this feature, just because it forces them to install Guzzle (instead of using their favorite HTTP client which is why I originally started HTTPlug/PSR-18)
Any thoughts?
Also, @SpencerMalone feel free to chime in!
Thanks again for the great PR!
| @@ -0,0 +1,290 @@ | |||
| Async Processing | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate writing an awesome documentation.
However, the docs have been migrated here: https://github.com/twirphp/twirphp.github.io
Could you send a PR there? It's in markdown format though.
I'm happy to merge this PR without the docs PR though, so it's not a blocker.
| return $hat; | ||
| } | ||
|
|
||
| public function MakeHatAsync(array $ctx, Size $size): PromiseInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if adding a separate interface would help. Clients could always implement that interface, but servers wouldn't have to.
I agree, forcing service implementations to add this seems unnecessary and creates friction.
Thoughts @winmutt ?
@SpencerMalone I don't think Guzzle is a hard dependency. There is a fallback mechanism to PSR-18. |
|
Ah, I was thinking the inclusion of the guzzle promise interface in |
Overview
This PR introduces asynchronous request processing for Twirp PHP clients using Guzzle's promise framework that adheres to PSR-7 standards.
Features
✅ All RPC methods now have async variants (e.g.,
MethodNameAsync())✅ Returns promises for non-blocking HTTP requests
✅ Supports concurrent requests using Guzzle promises
✅ Automatic fallback to synchronous execution for non-Guzzle clients
✅ Full backward compatibility - all existing tests pass
✅ Both Protobuf and JSON clients support async operations
Implementation Details
doRequestAsync()abstract methoddocs/advanced/async-processing.rstwith examples and best practicesExample Usage
Testing
All existing tests pass (35 unit tests + 18 integration tests).
Breaking Changes
None - this is a fully backward-compatible addition.
Documentation
See
docs/advanced/async-processing.rstfor comprehensive documentation.Changes Summary