laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.22k stars 10.91k forks source link

Undefined index: laravel_data #41872

Closed glamorous closed 2 years ago

glamorous commented 2 years ago

Description:

For unknown reason horizon jobs are failing due to this:

ErrorException: Undefined index: laravel_data in PATH_WHERE_CODE_LIVES/vendor/laravel/framework/src/Illuminate/Http/Client/PendingRequest.php:972

The code that is causing this:

$pendingRequest = $this->createPendingRequest($headers, $timeout, $clientRequest);
$response = $pendingRequest->get($url, $queryParams); // This is where I exception starts bubbling into the framework.

with the custom function createPendingRequest which is just calling the HTTP facade.

private function createPendingRequest(array $headers, int $timeout, &$clientRequest = null): PendingRequest
{
        $options = [
            'connect_timeout' => config('guzzle.connect_timeout'),
            'timeout' => $timeout,
        ];

        return Http::withHeaders($headers)
            ->withOptions($options)
            ->beforeSending(function (ClientRequest $request) use (&$clientRequest) {
                $clientRequest = $request;
            });
}

Steps To Reproduce:

Happens randomly. But wen it starts, it only stops by doing a horizon:terminate

This is a very hard thing for us to figure out why this is failing inside the framework. If it's not a bug but a problem on infrastructure level that "laravel_data" would be unknown, I can use some "guidance" or ideas which will cause this to happen.

driesvints commented 2 years ago

Hi there,

Thanks for reporting but it looks like this is a question which can be asked on a support channel. Please only use this issue tracker for reporting bugs with the library itself. If you have a question on how to use functionality provided by this repo you can try one of the following channels:

However, this issue will not be locked and everyone is still free to discuss solutions to your problem!

Thanks.

glamorous commented 1 year ago

For anyone that having the same problem. Look for more information in the post on laracast forum: https://laracasts.com/discuss/channels/laravel/undefined-index-laravel-data-in-pendingrequest?page=1&replyId=853723. Summary: 99% chance is caused by a memory leak in queues together with guzzle and New Relic. As a solution: let your queues stop/restart after a amount of jobs or time.

LMCom commented 10 months ago

Currently running 9.52.16 and I can reproduce the behaviour which really appears to be a bug, although not a memory leak.

I need to add some logging handlers to the HTTP client and pass it to some API library. After a frustrating bug search, I ended up with this minimal code which throws an exception but does not look obviously wrong:

$client = Illuminate\Support\Facades\Http::buildClient();
$response = $client->get('https://google.com')

Result:

 WARNING  Undefined array key "laravel_data" in vendor/laravel/framework/src/Illuminate/Http/Client/PendingRequest.php on line 1200.

   TypeError  Illuminate\Http\Client\Request::withData(): Argument #1 ($data) must be of type array, null given, called in vendor/laravel/framework/src/Illuminate/Http/Client/PendingRequest.php on line 1200.

The line: https://github.com/laravel/framework/blob/082345d76fc6a55b649572efe10b11b03e279d24/src/Illuminate/Http/Client/PendingRequest.php#L1200

So the key laravel_data does not seem to be set.

This could be done in the request, like so:

$response = $client->get('https://google.com', ['laravel_data' => []])

That code works. But, as I wrote, I am just passing the client to a library, and that library does not add laravel_data to the request, of course.

What do you say, @glamorous? Anyone else has some ideas?

glamorous commented 10 months ago

I gave up to try to find the exact reason why it was not set anymore by Laravel. The solution that I posted here is just a workaround and that way I didn't had the error anymore. But I agree, it should not be "fixed" like that, but prevented in code that that laravel_data was not present.

lk77 commented 10 months ago

the buildClient is not documented anywhere, so i suppose it should not be used

LMCom commented 10 months ago

What's an alternative way to get a PSR-7 compatible client without requiring a concrete library like GuzzleHttp? The Http facade gives the impression that it wants to do just that and abstract the specifics away. I totally support that. But if it cannot give us a client instance, it falls short.

lk77 commented 10 months ago

For me the buildClient method clearly seems to be an internal method that should not be used, it's not documented anywhere and could surely be made protected at any time

LMCom commented 10 months ago

I did not deny that. Your assumption seems to be plausible.

glamorous commented 10 months ago

You don't need to use the buildClient to have the error. As you can see in my orignal post, I just use the documentated methods which will return a PendingRequest.

lk77 commented 10 months ago

@glamorous for me your code works just fine :

if i dump the $options i have the laravel_data key

array:11 [
  "http_errors" => false
  "timeout" => 3000
  "laravel_data" => []
  "on_stats" => [...]
  "synchronous" => true
  "handler" => GuzzleHttp\HandlerStack^ {#773
    -handler: [...]
    -stack: array:7 [
      [...]
    ]
    -cached: [...]
  }
  "cookies" => [...]
  "allow_redirects" => array:5 [
    "max" => 5
    "protocols" => array:2 [
      0 => "http"
      1 => "https"
    ]
    "strict" => false
    "referer" => false
    "track_redirects" => false
  ]
  "decode_content" => true
  "verify" => true
  "idn_conversion" => false
]

calling get() successfully set the laravel_data in the sendRequest() function

perhaps this was fixed

glamorous commented 10 months ago

No, the thing is that is worked fined for me as well. The problem only occured after certain time. It was fixed like I suggested to close the queue after some jobs or after some time to avoid memory issues so I my understanding was that it's caused by memory issues. So after a restart of queue/daemon, the above code worked just fine but after memory issues, the same code causes issues (laravel data missing)

func0der commented 4 months ago

Why is this closed?

$client = Illuminate\Support\Facades\Http::buildClient();
$response = $client->get('https://google.com')

This perfectly reproduces the problem even with version 10.48.8.

If the method should not be used, it should be marked as @internal. As long as it is not marked as such, it is public API of the framework.

LMCom commented 4 months ago

The best solution would still be to fix the bug with the unset laravel_data. Like I wrote, I want to use a generic http client, and I do not see any other way to get one. Like with other parts of Laravel, Illuminate\Support\Facades\Http looks like an intended way to get some provided http client, not necessarily Guzzle. Or is there a reason for an exception from Laravel conventions here?

func0der commented 4 months ago

It would only be logical if you could get a PSR client from Laravel somehow. Currently you kinda get a broken one and you can not simply fix it in the container or anywhere else, except for in the software itself.

@driesvints Please re-open the issue.

driesvints commented 4 months ago

As said before, we don't intend buildClient to be used directly. It depends on data added in sendRequest before it can function properly. You can always attempt a PR to improve this if you want.

func0der commented 4 months ago

As said before, we don't intend buildClient to be used directly.

You can not have anything not intended to be used publicly available.

It depends on data added in sendRequest before it can function properly.

Since one can not or better has not enforced that behaviour, one should not rely on it. The flow of the code is implicit not explicit.

You can always attempt a PR to improve this if you want.

Done.

glamorous commented 4 months ago

Hi @func0der, I don't believe your solution will prevent the error from happening as you can see in my original post it happened sometimes without the (public) use of buildClient or getClient. I "fixed" it in my projects by restarting the queue more often than wanted so there would be no memory leaks which in my case where responsible for the HTTP client not setting the laravel_data in the request that was created.

func0der commented 4 months ago

@glamorous I do not know how restarting a queue ties into this. Could you elaborate?

For the moment, I think we need to focus on a very small reproducable example here to even remotely address this. When we got a solution, we can test it in other contexts, which honestly should not even care about what is happening in the client itself. That's why we have it :D But we will see.

There are obviously multiple angles to this. I can not get a clear answer from the Laravel project to what is the goal here, apart from that it is an internal function. So I have to go on that and make those methods inaccessible from the outside.

glamorous commented 4 months ago

I understand, but the original problem/bug happend with normal usage of the client (not with those methods you mentioned). Problems happend only in queue environments and after reading blog post from @themsaid regarding strang behaviorus when you're having memory leaks in the queue I decided to take action on that and as a "result" the issue didn't happened for me anymore. So my best guess was that the memory issue was responsible for the missing "laravel_data" key in the session for outgoing calls.