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

[10.x] allow $sleepMilliseconds parameter receive a Closure in retry method from PendingRequest #46653

Conversation

williamtrevisan
Copy link
Contributor

@williamtrevisan williamtrevisan commented Mar 31, 2023

The retry helper accepts either an int or a Closure. When passing a Closure object, the $attempts and the $exception would be passed, allowed users to implement their own backoff algorithm. The retry helper is used in the Pending Request class to perform request retries as well, but in this specific local, the parameters are sent after being informed by retry method that belongs to the same class mentioned, and in this method, is not accepted to inform $sleepMilliseconds as a Closure.

This pull request will allows you to inform $sleepMilliseconds parameter as a Closure or int, opening the possibility to create your own backoff algorithm for retry requests.

Example use case

Imagine a scenario where it is necessary to perform a backoff when the exception returned is of type Request Timeout (408), with this modification it would be possible to implement a dynamic strategy as follows:

Http::retry(
  times: 2,
  sleepMilliseconds: function (int $attempt, \Throwable $exception): int {
    if ($exception->getCode() === \Illuminate\Http\Response::HTTP_REQUEST_TIMEOUT) {
      return $attempt * 300000; // five minutes in milliseconds
    }
                
    return 0;
  }
);

@williamtrevisan williamtrevisan marked this pull request as ready for review March 31, 2023 21:55
@williamtrevisan williamtrevisan changed the title [10.x] allow $sleepMilliseconds receive closure in retry method from PendingRequest [10.x] allow $sleepMilliseconds receive Closure in retry method from PendingRequest Mar 31, 2023
@williamtrevisan williamtrevisan changed the title [10.x] allow $sleepMilliseconds receive Closure in retry method from PendingRequest [10.x] allow $sleepMilliseconds parameter receive a Closure in retry method from PendingRequest Mar 31, 2023
@taylorotwell taylorotwell merged commit fdfcf9e into laravel:10.x Apr 2, 2023
@williamtrevisan williamtrevisan deleted the feat/allow-sleep-milliseconds-receive-closure branch April 3, 2023 11:06
$this->factory->assertSentCount(3);

// Make sure was waited 300ms for the first two attempts
$this->assertEqualsWithDelta(0.3, microtime(true) - $startTime, 0.03);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could fail in slow machines

Copy link
Contributor Author

@williamtrevisan williamtrevisan May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know another approach that I can use here to test this? Or maybe it's not necessary test that functionality knowing that is already tested in retry helper scope? Basically I have used the same approach that exists there

@CLL-GTA
Copy link

CLL-GTA commented May 24, 2023

Would it be possible to do this on 9.x as well? I don't think there is an issue as the retry() helper already accepts a Closure for $sleepMilliseconds

@williamtrevisan
Copy link
Contributor Author

Would it be possible to do this on 9.x as well? I don't think there is an issue as the retry() helper already accepts a Closure for $sleepMilliseconds

Yes, it would. I'll open a PR to implement that there.

@williamtrevisan
Copy link
Contributor Author

williamtrevisan commented May 25, 2023

Would it be possible to do this on 9.x as well? I don't think there is an issue as the retry() helper already accepts a Closure for $sleepMilliseconds

Yes, it would. I'll open a PR to implement that there.

Well, I have opened a PR to make it possible in 9.x, but folks from laravel core said that 9.x is not open for new features, and people needs to upgrade to 10.x. You can see this here

@CLL-GTA
Copy link

CLL-GTA commented May 25, 2023

Well, I have opened a PR to make it possible in 9.x, but folks from laravel core said that 9.x is not open for new features, and people needs to upgrade to 10.x. You can see this #47213

Well, that's disappointing, but thanks for trying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants