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

Query Cache mangled if saved by-reference #6552

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Conversation

Slamdunk
Copy link
Contributor

@Slamdunk Slamdunk commented Oct 17, 2024

Bug emerged in #6510 (comment)

The current implementation of query cache relies on the cache not saved by-reference; the bug has never been seen before because by default new ArrayAdapter() saves the cache with serialization, hence breaking the by-reference pointer.

Once the by-reference tecnique is used, two issues pop up:

  1. \Doctrine\DBAL\Cache\ArrayResult::$num is never reset, so once it gets incremented in the first \Doctrine\DBAL\Cache\ArrayResult::fetch call, the following calls will always fail
  2. Even considering fixing the $num property reset, a manual call on \Doctrine\DBAL\Result::free will by cascade call the \Doctrine\DBAL\Cache\ArrayResult::free method erasing all the saved results

I think that the ArrayResult implementation is not the culprit, but rather the #6510 giving to the cache backend the internal object by reference instead of giving it a copy.

@derrabus
Copy link
Member

derrabus commented Oct 17, 2024

We can probably solve this by cloning the ArrayResult before storing and returning it. That being said, you really should not use this kind of cache in production.

@Slamdunk
Copy link
Contributor Author

That bein said, you really should not use this kind of cache in production.

I disagree: we have in production many long-running processes where this kind of cache is enough and more performant than the alternatives

@derrabus
Copy link
Member

I mean, you do you, but you're like asking for the kind of trouble that you see here. No other cache would behave this way. What you call "the by-reference tecnique" is simply put non-standard behavior.

@derrabus derrabus added this to the 4.2.2 milestone Oct 21, 2024
@derrabus derrabus merged commit 8f60369 into doctrine:4.2.x Oct 21, 2024
90 of 91 checks passed
@derrabus
Copy link
Member

Thank you.

@derrabus derrabus changed the title [Bug] Query Cache mangled if saved by-reference Query Cache mangled if saved by-reference Oct 21, 2024
@Slamdunk Slamdunk deleted the bug_6510 branch October 21, 2024 08:03
derrabus added a commit that referenced this pull request Oct 21, 2024
* 4.2.x:
  Create stubs instead of mocks (#6564)
  [Bug] Query Cache mangled if saved by-reference (#6552)
  test: remove ->expects(self::any())
  Fix typo in PostgreSql documentation reference
  fix
  Acknowledge the existence of 3.10 (#6553)
  test: cover nested transactions
derrabus added a commit that referenced this pull request Oct 21, 2024
* 4.3.x:
  Create stubs instead of mocks (#6564)
  [Bug] Query Cache mangled if saved by-reference (#6552)
  test: remove ->expects(self::any())
  Fix typo in PostgreSql documentation reference
  fix
  Acknowledge the existence of 3.10 (#6553)
  Simplify tracking implicitly created indexes
  Remove handling unuique constraint column names as associative array (#6549)
  test: cover nested transactions
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.

2 participants