Cache ipCheck (2.8) #22819

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

Closed

gonzalovilaseca wants to merge 1 commit into symfony:2.8 from gonzalovilaseca:patch-2

Contributor

gonzalovilaseca commented May 21, 2017 •

edited

Loading

In our app we use trusted proxies. Using Blackfire we found IpUtils::checkIp was being called 454 times taking 3.15ms.
Caching the result saves those 3ms.

Q	A
Branch?	2.8
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

carsonbot added the Status: Needs Review label

sstok suggested changes

View reviewed changes

src/Symfony/Component/HttpFoundation/Request.php Outdated

@@ @@ -213,6 +213,11 @@ class Request @@
                   protected $defaultLocale = 'en';
                   /**
+                   * @var boolean

Contributor

sstok May 21, 2017

This needs to be bool, see fabbot.io patch

src/Symfony/Component/HttpFoundation/Request.php Outdated

+                          return false;
+                      }
+                      if (!isset($this->validTrustedProxyIP)) {

Contributor

sstok May 21, 2017

It is possible that self::$trustedProxies is changed between method calls. Make sure to reset the cached result in setTrustedProxies and that this is tested.

Member

nicolas-grekas commented May 21, 2017

shouldnt this fix be submitted against the 2.8 branch?

nicolas-grekas added this to the 2.8 milestone

Contributor

tgalopin commented Jun 4, 2017 •

edited

Loading

I stumbled upon the same issue. Shouldn't the cache be stored statically in IpUtils instead of in the request? It makes more sense to me, as the request is a representation of a HTTP object, using it to store cache feels wrong. Moreover, using a cache in IpUtils ensure a usage outside of the Request would still be cached.

WDYT?

Member

fabpot commented Jun 4, 2017

Indeed, a cache in IpUtils seems a better idea.

Contributor

tgalopin commented Jun 4, 2017

@gonzalovilaseca do you want to do it? I can if you don't have time right now.

robfrawley reviewed

View reviewed changes

src/Symfony/Component/HttpFoundation/Tests/RequestTest.php Outdated

+                      $request = new Request(array(), array(), array(), array(), array(), array('REMOTE_ADDR' => '8.8.8.8'));
+                      Request::setTrustedProxies(array('8.8.8.8'), Request::HEADER_FORWARDED);
+                      $this->assertEquals(true, $request->isFromTrustedProxy());

Contributor

robfrawley Jun 4, 2017 •

edited

Loading

$this->assertTrue($this->isFromTrustedProxy());

Also seen below in testIsFromTrustedProxyCacheIsInValidatedOnSetTrustedProxies().

Contributor Author

gonzalovilaseca commented Jun 4, 2017

@tgalopin Sure, I'll do it, will be ready by tuesday

stloyd suggested changes

View reviewed changes

src/Symfony/Component/HttpFoundation/Tests/RequestTest.php Outdated


		Request::setTrustedProxies(array('5.5.5.5'), Request::HEADER_FORWARDED);

		$this->assertEquals(false, $request->isFromTrustedProxy());

Contributor

stloyd Jun 5, 2017

assertFalse()

src/Symfony/Component/HttpFoundation/Tests/RequestTest.php Outdated

+                  public function testIsFromTrustedProxyReturnsFalseIfNoTrustedProxies()
+                  {
+                      $request = new Request();
+                      $this->assertEquals(false, $request->isFromTrustedProxy());

Contributor

stloyd Jun 5, 2017

assertFalse()

src/Symfony/Component/HttpFoundation/Request.php Outdated

+                      }
+                      $remoteAddr = $this->server->get('REMOTE_ADDR');
+                      if (!array_key_exists($remoteAddr, self::$validTrustedProxyIP) || null === self::$validTrustedProxyIP[$remoteAddr]) {

Contributor

stloyd Jun 5, 2017

Can be simply: !isset(self::$validTrustedProxyIP[$remoteAddr]) cause you do check for null anyway.

Contributor Author

gonzalovilaseca commented Jun 6, 2017

I've updated the PR moving the cache to IpUtils, I think that an IpUtils decorator would be a better solution, but being called statically from other places it would mean a bit of refactoring, should I go the decorator way or is this ok?

Feel free to improve the naming of the new methods, naming is not my strength, even less with jet lag :-).

fabpot requested changes

View reviewed changes

src/Symfony/Component/HttpFoundation/IpUtils.php Outdated

@@ @@ -19,6 +19,11 @@ @@
               class IpUtils
               {
                   /**
+                   * @var array
+                   */
+                  protected static $checkedIps = array();

Member

fabpot Jun 6, 2017

Should be private

src/Symfony/Component/HttpFoundation/IpUtils.php Outdated

@@ @@ -61,6 +66,60 @@ public static function checkIp($requestIp, $ips) @@
                    */
                   public static function checkIp4($requestIp, $ip)
                   {
+                      return self::checkIpInternal($requestIp, $ip, 'self::checkIp4Internal');

Member

fabpot Jun 6, 2017

I would not delegate to another method but just copy/paste the cache logic in the methods directly.

fabpot reviewed

View reviewed changes

src/Symfony/Component/HttpFoundation/IpUtils.php Outdated

@@ @@ -61,6 +66,50 @@ public static function checkIp($requestIp, $ips) @@
                    */
                   public static function checkIp4($requestIp, $ip)
                   {
+                      $cacheKey = $requestIp.'-'.$ip;
+                      if (!array_key_exists($cacheKey, self::$checkedIps)) {
+                          self::$checkedIps[$cacheKey] = self::checkIp4Internal($requestIp, $ip);

Member

fabpot Jun 6, 2017

I would inline the checkIp4Internal method as well.

fabpot requested changes

View reviewed changes

src/Symfony/Component/HttpFoundation/IpUtils.php Outdated

@@ @@ -19,6 +19,11 @@ @@
               class IpUtils
               {
                   /**
+                   * @var array
+                   */

Member

fabpot Jun 6, 2017

phpdoc should be removed

src/Symfony/Component/HttpFoundation/IpUtils.php Outdated

-                          return false;
-                      }
+                      $cacheKey = $requestIp.'-'.$ip;
+                      if (!array_key_exists($cacheKey, self::$checkedIps)) {

Member

fabpot Jun 6, 2017

I would return early:

if (array_key_exists($cacheKey, self::$checkedIps)) {
    return self::$checkedIps[$cacheKey];
}

I would even change arra_key_exists() to isset()

fabpot reviewed

View reviewed changes

src/Symfony/Component/HttpFoundation/IpUtils.php Outdated

@@ @@ -61,26 +63,31 @@ public static function checkIp($requestIp, $ips) @@
                    */
                   public static function checkIp4($requestIp, $ip)
                   {
+                      $cacheKey = $requestIp.'-'.$ip;
+                      if (isset($cacheKey, self::$checkedIps)) {

Member

fabpot Jun 6, 2017

Looks wrong to me

Member

fabpot commented Jun 6, 2017

@gonzalovilaseca Can you change the branch of the PR to 2.7?

Contributor Author

gonzalovilaseca commented Jun 6, 2017

@fabpot 2.7 or 2.8? (nicolas mentioned 2.8)

gonzalovilaseca changed the base branch from master to 2.7

June 6, 2017 19:23

Member

fabpot commented Jun 6, 2017

I would say 2.7.

gonzalovilaseca changed the base branch from 2.7 to 2.8

June 6, 2017 19:25

gonzalovilaseca changed the base branch from 2.8 to 2.7

June 6, 2017 19:26

Contributor Author

gonzalovilaseca commented Jun 6, 2017

@fabpot IpUtils changed from 2.7 to 2.8, should I do a PR for each branch?

Member

fabpot commented Jun 6, 2017

That would be wonderful, thanks.


          Cache ipCheck

09296ee

gonzalovilaseca changed the base branch from 2.7 to 2.8

June 7, 2017 19:13

gonzalovilaseca mentioned this pull request

Cache ipCheck (2.7) #23098

Merged

Contributor Author

gonzalovilaseca commented Jun 7, 2017

2.7 version here: #23098

gonzalovilaseca changed the title ~~Cache ipCheck~~ Cache ipCheck (2.8)

fabpot closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Needs Review