Skip to content

Add BC layer to handle old objects already present in cache #45876

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
nicolas-grekas merged 1 commit into from
Mar 29, 2022
Merged

Add BC layer to handle old objects already present in cache #45876

nicolas-grekas merged 1 commit into from
Mar 29, 2022

Conversation

@Seldaek
Copy link
Member

@Seldaek Seldaek commented Mar 29, 2022 •
edited
Loading

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Refs #44996
License MIT
Doc PR symfony/symfony-docs#...

When reloading an old RateLimiter instance from cache with the new code I did get an exception because windowEndAt was a string and getHitCount does windowEndAt - intervalInSeconds, and string - int fails.

The reason is the new code assigns the id to windowEndAt via:

        $pack = key($data);
        $this->windowEndAt = $data[$pack];

I dumped $this and $data in __unserialize just FYI and you get this:

object(Symfony\Component\RateLimiter\Policy\SlidingWindow)[959]
  private 'id' => null
  private 'hitCount' => int 0
  private 'hitCountForLastWindow' => int 0
  private 'intervalInSeconds' => null
  private 'windowEndAt' => null

array (size=5)
  '�Symfony\Component\RateLimiter\Policy\SlidingWindow�id' => string 'login_per_ip-127.0.0.1' (length=22)
  '�Symfony\Component\RateLimiter\Policy\SlidingWindow�hitCount' => int 2
  '�Symfony\Component\RateLimiter\Policy\SlidingWindow�intervalInSeconds' => int 60
  '�Symfony\Component\RateLimiter\Policy\SlidingWindow�hitCountForLastWindow' => int 0
  '�Symfony\Component\RateLimiter\Policy\SlidingWindow�windowEndAt' => float 1648544909.4762

I only tested the code in real conditions for SlidingWindow, but I do believe/hope the same applies for the other two policies.

@carsonbot carsonbot added this to the 5.4 milestone Mar 29, 2022
@Seldaek Seldaek mentioned this pull request Mar 29, 2022
Merged
Nyholm
Nyholm reviewed Mar 29, 2022
View reviewed changes
Seldaek
Seldaek commented Mar 29, 2022
View reviewed changes
Seldaek
Seldaek commented Mar 29, 2022
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Mar 29, 2022
View reviewed changes
src/Symfony/Component/RateLimiter/Policy/TokenBucket.php Outdated
$this->tokens = $data[1];
$this->timer = $data[2];
$this->burstSize = $data[3];
$this->stringRate = $data[4];
Copy link
Member

@nicolas-grekas nicolas-grekas Mar 29, 2022

Choose a reason for hiding this comment

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

actualy let's remove this one, remove wakeup and move its logic here

Copy link
Member

@nicolas-grekas nicolas-grekas Mar 29, 2022

Choose a reason for hiding this comment

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

argh no! that'd break compat with php < 7.4

Copy link
Member

@nicolas-grekas nicolas-grekas Mar 29, 2022

Choose a reason for hiding this comment

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

$this->rate = Rate::fromString($data[4]); then

Copy link
Member Author

@Seldaek Seldaek Mar 29, 2022

Choose a reason for hiding this comment

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

Oh yeah missed that the stringRate prop does not exist :)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 29, 2022

Thank you @Seldaek.

@nicolas-grekas nicolas-grekas merged commit 45d0023 into symfony:5.4 Mar 29, 2022
@Seldaek Seldaek deleted the patch-21 branch March 29, 2022 10:10
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

@Nyholm Nyholm Nyholm left review comments

Assignees

No one assigned

Labels

Bug Status: Reviewed

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

4 participants

@Seldaek @nicolas-grekas @Nyholm @carsonbot