Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Cache][WebProfiler][3.3] fix collecting cache stats with sub-requests #26080

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

Closed
wants to merge 1 commit into from

Conversation

dmaicher
Copy link
Contributor

@dmaicher dmaicher commented Feb 7, 2018

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

When performing sub-requests the CacheDataCollector will get the calls from the TraceableAdapter per request. So calling it multiple times was always overwriting the previously collected calls with the latest calls.

So when no cache calls were performed during a sub-request all collected calls from the master request were just lost and the panel did not show anything.

@Nyholm seems you added this specific code? What was the motivation to clear directly after getting the calls? Just prevent a memory leak?

As of 3.4+ there is a reset method and with this additional changeset we can therefore easily reset the TraceableAdapter?

diff --git a/src/Symfony/Component/Cache/Adapter/TraceableAdapter.php b/src/Symfony/Component/Cache/Adapter/TraceableAdapter.php
index e8563521ba..98d0e52693 100644
--- a/src/Symfony/Component/Cache/Adapter/TraceableAdapter.php
+++ b/src/Symfony/Component/Cache/Adapter/TraceableAdapter.php
@@ -204,11 +204,12 @@ class TraceableAdapter implements AdapterInterface, PruneableInterface, Resettab
 
     public function getCalls()
     {
-        try {
-            return $this->calls;
-        } finally {
-            $this->calls = array();
-        }
+        return $this->calls;
+    }
+
+    public function clearCalls()
+    {
+        $this->calls = array();
     }
 
     protected function start($name)
diff --git a/src/Symfony/Component/Cache/DataCollector/CacheDataCollector.php b/src/Symfony/Component/Cache/DataCollector/CacheDataCollector.php
index 62d502f01f..ceef45aa0b 100644
--- a/src/Symfony/Component/Cache/DataCollector/CacheDataCollector.php
+++ b/src/Symfony/Component/Cache/DataCollector/CacheDataCollector.php
@@ -57,8 +57,7 @@ class CacheDataCollector extends DataCollector implements LateDataCollectorInter
     {
         $this->data = array();
         foreach ($this->instances as $instance) {
-            // Calling getCalls() will clear the calls.
-            $instance->getCalls();
+            $instance->clearCalls();
         }
     }

See PR for 3.4+: #26082

@Nyholm
Copy link
Member

Nyholm commented Feb 7, 2018

Thank you for this PR.

I cannot recall why, sorry. But I think it makes sense to not clear the calls in getCalls. Especially since we have the scenario/bug you describe.
Yes, please do make a PR to allow us to reset the TraceableAdapter.

@dmaicher
Copy link
Contributor Author

dmaicher commented Feb 7, 2018

Ok @Nyholm 👍 Added the follow up PR for 3.4+ here: #26082

@dmaicher dmaicher changed the title [Cache][WebProfiler] fix collecting cache stats with sub-requests [Cache][WebProfiler][3.3] fix collecting cache stats with sub-requests Feb 7, 2018
@nicolas-grekas
Copy link
Member

Let's close this one in favor of #26082, isn't it? 3.3 is not maintained anymore.

@dmaicher
Copy link
Contributor Author

dmaicher commented Feb 7, 2018

@nicolas-grekas ah true missed that 👍

@dmaicher dmaicher closed this Feb 7, 2018
@dmaicher dmaicher deleted the fix-issue-23820 branch February 7, 2018 20:03
symfony-splitter pushed a commit to symfony/cache that referenced this pull request Feb 11, 2018
…sub-requests + allow clearing calls (dmaicher)

This PR was merged into the 3.4 branch.

Discussion
----------

[Cache][WebProfiler][3.4] fix collecting cache stats with sub-requests + allow clearing calls

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#23820
| License       | MIT
| Doc PR        | -

This is a follow-up PR for symfony/symfony#26080. It additionally adds the reset behavior for the `TraceableAdapter` back.

Commits
-------

132bba6 [Cache][WebProfiler] fix collecting cache stats with sub-requests + allow clearing calls
nicolas-grekas added a commit that referenced this pull request Feb 11, 2018
…sub-requests + allow clearing calls (dmaicher)

This PR was merged into the 3.4 branch.

Discussion
----------

[Cache][WebProfiler][3.4] fix collecting cache stats with sub-requests + allow clearing calls

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23820
| License       | MIT
| Doc PR        | -

This is a follow-up PR for #26080. It additionally adds the reset behavior for the `TraceableAdapter` back.

Commits
-------

132bba6 [Cache][WebProfiler] fix collecting cache stats with sub-requests + allow clearing calls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.