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

History middleware doesn't work with MultiRegionClient #2574

Closed
joetannenbaum opened this issue Nov 20, 2022 · 7 comments
Closed

History middleware doesn't work with MultiRegionClient #2574

joetannenbaum opened this issue Nov 20, 2022 · 7 comments
Assignees
Labels
feature-request A feature should be added or improved. needs-review p2 This is a standard priority issue

Comments

@joetannenbaum
Copy link

Describe the bug

I'm trying to get the history middleware to work with the MultiRegionClient, but it's missing a required argument for the start method. I've tried to track down where these arguments get passed in, but I haven't been able to locate it.

Expected Behavior

I expect to be able to view the history of requests passed through the client.

Current Behavior

I encounter the following error:

TypeError: Aws\History::start(): Argument #2 ($req) must be of type Psr\Http\Message\RequestInterface, null given, vendor/aws/aws-sdk-php/src/Middleware.php on line 329

Reproduction Steps

<?php

use Aws\History;
use Aws\Middleware;
use Aws\MultiRegionClient;

$client = new MultiRegionClient([
  'service' => 'ec2',
  'version' => 'latest',
]);

$history = new History(20);

$client->getHandlerList()->appendSign(Middleware::history($history));

$result = $client->describeSecurityGroups([
  '@region' => 'us-west-2',
]);

Possible Solution

No response

Additional Information/Context

No response

SDK version used

3.246.1

Environment details (Version of PHP (php -v)? OS name and version, etc.)

PHP 8.1.12, Mac, Laravel Valet

@joetannenbaum joetannenbaum added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 20, 2022
@yenfryherrerafeliz yenfryherrerafeliz self-assigned this Nov 20, 2022
@yenfryherrerafeliz yenfryherrerafeliz added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Nov 20, 2022
@yenfryherrerafeliz
Copy link
Contributor

Hi @joetannenbaum, thanks for opening this issue. I can confirm the error reported and we are currently investigating this. I will get back to you as soon as I get more updates regarding.

Thanks!

@joetannenbaum
Copy link
Author

@yenfryherrerafeliz if there's any way I can help let me know, thanks so much! If you'd like to point me in the right direction I'd be happy to poke around.

@yenfryherrerafeliz
Copy link
Contributor

Hi @joetannenbaum, sorry for the delay responding to this. After a deeper look I found that, the reason why this issue happens is because, the middlewares added to the multi region client are invoked in a scope where the request is not built yet. To better explain this, lets revisit some components; HandlerList, that is a set of functions that shares data in between each other, and are invoked in order, and wheres the last function is the handler; The "handler" is a function responsible of returning a promise, and that when fulfilled it will return a object as result of type "Aws\ResultInterface". So, when we add a middleware, we are adding it to an one of the steps of a HandlerList instance. Now, lets check what is being executed when we call "$client->describeSecurityGroups", basically, describeSecurityGroups is considered a magic function and it is handled here, then, after the validations in there, we should end up in this line, and as we can see, it is calling the function ""$command->getHandlerList()->resolve()", which basically composes all the middlewares and the handler into a single callable function, which is called in the next line. So, what is the problem here?, the problem is that the middleware we have added to the handler list of the client, is being called before the handler here, and an exception is being thrown in that call, because there is not request built at that time; we can see in the handler logic that, there, is where the needed client "EC2Client" is created, and the command to be executed, which is gotten from that client, and then it returns $this->executeAsync($command), and there, is when and where the request starts to be built, based on the middlewares and handler that comes with the client created "EC2Client in this case".

So, to summarize this problem:

  • We create the instance of the client:
    • $client = new MultiRegionClient([
           'service' => 'ec2',
           'version' => 'latest'
       ]);
  • We append the middleware:
    • $client->getHandlerList()->appendSign(Middleware::history($history), 'history-middleware');
  • We make the call:
    • $client->describeSecurityGroups([
            '@region' => 'us-west-2',
       ]);
  • Execution:
    Note: I am skipping some steps, but we will end up at executeAsync function.
    • When executeAsync is called the first time, it will compose the middlewares we have added to the MultiRegionClient, which in our case is just the history middleware, along with the handler function defined here, and as we already know the middleware function will be called first, and there, is where the exception will be thrown, also preventing the subsequent functions as the handler of the MultiRegionClient from being called.

Unfortunately at this time I do not have any workarounds at the top of my head, but I will bring this issue into the team tomorrow and I will provide updates accordingly.

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added needs-review and removed needs-triage This issue or PR still needs to be triaged. investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Nov 27, 2022
@joetannenbaum
Copy link
Author

@yenfryherrerafeliz wow thank you so much for this thorough explanation! I really appreciate you taking the time to explain the issue to me. Thank you for the continued updates.

@yenfryherrerafeliz yenfryherrerafeliz added the p2 This is a standard priority issue label Jan 2, 2023
@yenfryherrerafeliz yenfryherrerafeliz added feature-request A feature should be added or improved. and removed bug This issue is a bug. labels Aug 28, 2023
@yenfryherrerafeliz
Copy link
Contributor

Hi @joetannenbaum, again thanks for raising this issue. At this time, we have decided that we will not implement this feature.

Please, if you happen to need anything else that we may help with, feel free of opening a new issue.

Thanks!

Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

@yenfryherrerafeliz yenfryherrerafeliz closed this as not planned Won't fix, can't repro, duplicate, stale Jul 29, 2024
Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. needs-review p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

2 participants