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

Prevent Redis from crashing from key tracking invalidations #11814

Merged
merged 9 commits into from Feb 21, 2023

Conversation

madolson
Copy link
Contributor

@madolson madolson commented Feb 17, 2023

Resolves #11715.

There is a built in limit to client side tracking keys, which when exceeded will invalidate keys. This occurs in two places, one in the server cron and other before executing a command. If it happens in the second scenario, the invalidations will be queued for later since current client is set. This queue is never drained if a command is not executed (through call) such as a multi-exec command getting queued. This results in a later server assert crashing.

Related: #9422.

Release notes
Fix a crash that can occur when client side tracking keys are invalidated when the max number of tracked keys has been reached.

@ranshid
Copy link
Collaborator

ranshid commented Feb 17, 2023

@madolson. this solves the crash but not the problem right? we would still like to get invalidations when shrinking the invalidation table.

@ranshid
Copy link
Collaborator

ranshid commented Feb 17, 2023

@madolson. this solves the crash but not the problem right? we would still like to get invalidations when shrinking the invalidation table.

Oh - NVM. noticed we will still send the invalidations just not queue them...

src/tracking.c Outdated Show resolved Hide resolved
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

i see the original crash report is for 6.2, any reason not to take this fix there?
or maybe it requires a different fix since the infra was heavily modified?

tests/unit/tracking.tcl Show resolved Hide resolved
Copy link
Collaborator

@ranshid ranshid left a comment

Choose a reason for hiding this comment

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

LGTM

@ranshid
Copy link
Collaborator

ranshid commented Feb 19, 2023

i see the original crash report is for 6.2, any reason not to take this fix there? or maybe it requires a different fix since the infra was heavily modified?

@oranagra the crash is a result of the refactor which was made in 6.2.7: #9422
So yes it is required to be backported to 6.2

madolson and others added 2 commits February 19, 2023 11:09
Co-authored-by: Huang Zhw <huang_zhw@126.com>
@madolson
Copy link
Contributor Author

madolson commented Feb 21, 2023

When backporting to 6.2 and 7.0, we we needed the pause cron to be backported as well, I pulled out into this commit: madolson@a382b90.

We also need some indicator we are in the command, the current flag is based off of the blocking refactor. EDIT: I think this fits, dc742bf. I'm happy Ran did the refactoring :).

@madolson madolson added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 21, 2023
@oranagra
Copy link
Member

So the last commit you referred to is an alternative instead of relying on the new flag? Any reason you didn't merge the pr yet?

@madolson
Copy link
Contributor Author

So the last commit you referred to is an alternative instead of relying on the new flag?

Yes

@madolson madolson merged commit dca5927 into redis:unstable Feb 21, 2023
oranagra pushed a commit to oranagra/redis that referenced this pull request Feb 27, 2023
)

(cherry picked from commit f7150c45bc5d6f03c8ba86a9a9296d024c6848dc)
@oranagra oranagra mentioned this pull request Feb 27, 2023
oranagra pushed a commit to oranagra/redis that referenced this pull request Feb 27, 2023
@oranagra oranagra mentioned this pull request Feb 27, 2023
oranagra pushed a commit that referenced this pull request Feb 28, 2023
(cherry picked from commit f7150c45bc5d6f03c8ba86a9a9296d024c6848dc)
Mixficsol pushed a commit to Mixficsol/redis that referenced this pull request Apr 12, 2023
)

There is a built in limit to client side tracking keys, which when exceeded will invalidate keys. This occurs in two places, one in the server cron and other before executing a command. If it happens in the second scenario, the invalidations will be queued for later since current client is set. This queue is never drained if a command is not executed (through call) such as a multi-exec command getting queued. This results in a later server assert crashing.
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
)

There is a built in limit to client side tracking keys, which when exceeded will invalidate keys. This occurs in two places, one in the server cron and other before executing a command. If it happens in the second scenario, the invalidations will be queued for later since current client is set. This queue is never drained if a command is not executed (through call) such as a multi-exec command getting queued. This results in a later server assert crashing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[CRASH] server.c:2442 'listLength(server.tracking_pending_keys) == 0' is not true
4 participants