-
Notifications
You must be signed in to change notification settings - Fork 152
Standard Promises #71
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
Conversation
Will reopen after we clarify technical direction. |
Another way that was proposed to execute service contracts in an asynchronous manner was to use async web API, but there | ||
are number of problems with that approach: | ||
* Async web API allows execution of the same operation with different sets of arguments, but not different operations | ||
* Async web API was meant for execution big number of operations at the same time (thouthands) which is not the case |
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.
Actually, it's not only an import operation - place orders, shipments creation, invoicing, shipping rates retrieving.
design-documents/promises.md
Outdated
and to let client code to chain, pass and properly receive promises of results of operations. | ||
|
||
#### How will it look? | ||
There are to ways we can go about using promises for asynchronous execution of service contracts: |
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.
Probably, "two"
design-documents/promises.md
Outdated
```php | ||
interface SomeRepositoryInterface | ||
{ | ||
public function save(DTOInterface $dto): void; |
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.
What about existing repositories which returns DTOInterface
entity on save
method?
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.
They can, that's just an example meant to show that 2nd approach does not require a particular return type
design-documents/promises.md
Outdated
$saveOperation->then(function ($result) use (&$updated) { $updated = $result; }); | ||
|
||
//Waiting for all | ||
$saveOperation->wait(); |
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.
Should we call wait
for both async and sync approaches?
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.
you would call 'wait' to wait for an operation to complete if it's required, you can just provide a callback to 'then' method if you don't need waiting
@@ -0,0 +1,240 @@ | |||
### Why? | |||
Service contracts in the future will often be executed in an asynchronous manner |
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.
Please elaborate more on the purpose and provide examples where it can be used now. As I understand, the purpose is to allow call APIs in parallel when we need to call multiple APIs consequentially and as a result improve performance? Where promises will be used, do you propose public API to return promises?
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.
public API methods don't have to return promise since we'll be calling them via HTTP. If you mean that when a service contract implementation will actually be calling web API on another server then yes, if we go with the 1st route they'd have to return a promise
design-documents/promises.md
Outdated
|
||
### Implementation | ||
A wrapper around [Guzzle Promises](https://github.com/guzzle/promises) will be created to implement the APIs above. Guzzle Promises fit | ||
most important criteria - they allow synchronous execution as well as asynchronous. It's a mature |
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.
What are the benefits of the interface that would allow us to call something both ways?
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.
if a promise would only allow 'then' and 'otherwise' callback then all the client code using would have to be returning promises as well because there would be no way to actually wait for an async operation to finish
``` | ||
|
||
### Using promises for existing code | ||
We have a standard HTTP client - Magento\Framework\HTTP\ClientInterface, it can benefit from allowing async requests |
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.
Does it mean that we will have to change the interface? This might be backwards incompatible.
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.
we would have to add methods or add an interface that will extend the existing one
design-documents/promises.md
Outdated
|
||
#### How will it look? | ||
There are to ways we can go about using promises for asynchronous execution of service contracts: | ||
* Service interfaces themselves returning promises for client code to use |
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.
How will this work in the existing system? In web API for instance?
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.
web API controllers would just have to updated to expect promises, calling wait on them and then rendering results
fc15a42
to
0182b9d
Compare
Decided that futures serve us better |
@AlexMaxHorkun could you add async http interface description? @melnikovi , @akaplya do you have any comments/objections? |
Page described how to work with DeferredInterface introduced in magento/architecture#71
Standard promises to be used with asynchronous operations in Magento. Operations like calling a remote service or just sending an HTTP request asynchronously.