8000 [Messenger] Ceil waiting time when multiplier is a float on retry by WissameMekhilef · Pull Request #46797 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] Ceil waiting time when multiplier is a float on retry #46797

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 29, 2022
Merged

[Messenger] Ceil waiting time when multiplier is a float on retry #46797

merged 1 commit into from
Jun 29, 2022

Conversation

WissameMekhilef
Copy link
Contributor
@WissameMekhilef WissameMekhilef commented Jun 28, 2022 •
edited by nicolas-grekas
Loading
Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #46795
License MIT
Doc PR Not required

In the Messenger - MultiplierRetryStrategy the return type for the function getWaitingTime is int, however with the computation of that function we were returning a float, this is because float * int returns a float and the attribute $multiplier is a float.

With this fix, we are rounding up the delay and casting into an int.

@carsonbot carsonbot added this to the 6.1 milestone Jun 28, 2022
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@derrabus
Copy link
Member

Thank you. Can you please add a test that reproduces your bug?

@WissameMekhilef
Copy link
Contributor Author

Thank you. Can you please add a test that reproduces your bug?

Absolutely, I have updated the MultiplierRetryStrategyTest and added a few scenarios where the multiplier is a float.

Let me know if you need anything else

@derrabus
Copy link
Member

Thank you. However, the tests you've added pass even without your change.

@derrabus
Copy link
Member

One more thing, is Symfony 4.4 also affected by this bug? If so, we need to rebase your PR to the 4.4 branch.

@WissameMekhilef
Copy link
Contributor Author
WissameMekhilef commented Jun 28, 2022 •
edited
Loading

Thank you. However, the tests you've added pass even without your change.

That's not quite right, there is an issue with how the tests are currently set up. The testGetWaitTime function in the MultiplierRetryStrategy is wrong, the test function has this profile

public function testGetWaitTime(int $delay, int $multiplier, int $maxDelay, int $previousRetries, int $expectedDelay)

When the constructor of the MultiplierRetryStrategy has this profile

public function __construct(int $maxRetries = 3, int $delayMilliseconds = 1000, float $multiplier = 1, int $maxDelayMilliseconds = 0)

If I just add the scenario where the multiplier is a float and I don't change anything else, the test is failing.

Symfony\Component\Messenger\Tests\Retry\MultiplierRetryStrategyTest::testGetWaitTime with data set #15 (1000, 1.0, 5000, 0, 1000)
TypeError: Symfony\Component\Messenger\Tests\Retry\MultiplierRetryStrategyTest::testGetWaitTime(): Argument #2 ($multiplier) must be of type int, float given, called in /home/wissamemkh/Documents/Projects/symfony/.phpunit/phpunit-9.5-0/src/Framework/TestCase.php on line 1546

Now if apply the change in the \Symfony\Component\Messenger\Retry\MultiplierRetryStrategy::getWaitingTime function and I add the scenario where the multiplier is a float the test is still failing because of the profile of the test function. The test is failing for the same reason as I mentioned above.

So I need to change the profile of the test to reflect the actual parameters that are allowed by the MultiplierRetryStrategy constructor.

I think the issue needed 2 fix

  1. Cast the return value of the \Symfony\Component\Messenger\Retry\MultiplierRetryStrategy::getWaitingTime function to an int
  2. Update the profile of the \Symfony\Component\Messenger\Tests\Retry\MultiplierRetryStrategyTest::testGetWaitTime function to reflect the parameters allowed by the constructor of the strategy

Let me know if you still think that I missed something, I'll be happy to take another look.

Thanks! :)

@WissameMekhilef
Copy link
Contributor Author

One more thing, is Symfony 4.4 also affected by this bug? If so, we need to rebase your PR to the 4.4 branch.

I'll wait for you to confirm the comment is ok, then I'll rebase on Symfony 4.4 and change the base for the PR

@derrabus
Copy link
Member

Here's what I did: I checked out your branch and reverted your change to MultiplierRetryStrategy only. I ran the tests and they all pass, even your new ones. That tells me that your new tests don't reproduce the bug.

Bildschirmfoto 2022-06-28 um 16 31 41

@WissameMekhilef
Copy link
Contributor Author

Mmh that's interesting, I did some more digging and I found this PR that was accepted in April this year -- https://github.com/symfony/symfony/pull/45925/files#

Not sure how to reproduce this bug then

@norkunas norkunas mentioned this pull request Jun 29, 2022
Closed
@alamirault
Copy link
Contributor

Mmh that's interesting, I did some more digging and I found this PR that was accepted in April this year -- https://github.com/symfony/symfony/pull/45925/files#

Not sure how to reproduce this bug then

In this case, a deprecation was here before fix

Remaining self deprecation notices (1)

  1x: Implicit conversion from float 29.999989986419678 to int loses precision
    1x in SlidingWindowTest::testGetExpirationTime from Symfony\Component\RateLimiter\Tests\Policy

In our case no deprecation is triggered. And testGetWaitTime is KO only with strict_types=1. Not know if it's currently testable.

I confirm bug in 4.4

@nicolas-grekas nicolas-grekas modified the milestones: 6.1, 4.4 Jun 29, 2022
@nicolas-grekas nicolas-grekas changed the title [Messenger] - Retry strategy - Fix wrong return type [Messenger] Ceil waiting time when multiplier is a float Jun 29, 2022
@nicolas-grekas nicolas-grekas changed the title [Messenger] Ceil waiting time when multiplier is a float [Messenger] Ceil waiting time when multiplier is a float on retry Jun 29, 2022
@nicolas-grekas nicolas-grekas changed the base branch from 6.1 to 4.4 June 29, 2022 07:00
nicolas-grekas
nicolas-grekas approved these changes Jun 29, 2022
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I improved the tests to trigger the deprecation without the patch)

@nicolas-grekas
Copy link
Member

Thank you @WissameMekhilef.

@nicolas-grekas nicolas-grekas merged commit 99fa193 into symfony:4.4 Jun 29, 2022
This was referenced Jul 29, 2022
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@lyrixx lyrixx Awaiting requested review from lyrixx

@yceruto yceruto Awaiting requested review from yceruto

@wouterj wouterj Awaiting requested review from wouterj

@chalasr chalasr Awaiting requested review from chalasr

@dunglas dunglas Awaiting requested review from dunglas

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Bug Messenger Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@WissameMekhilef @carsonbot @derrabus @alamirault @nicolas-grekas
0