Fail Exec command in case a watched key is expired by perryitay · Pull Request #9194 · redis/redis

perryitay · 2021-07-04T12:45:48Z

There are two issues fixed in this Pr:

fixing issue [BUG?] EXEC commands succeeds when one of the queued commands uses a WATCHed expired key #9068, we want to fail the EXEC command in case there is a watched key that's logically expired but not yet deleted by active expire or lazy expire.
we saw that currently cache time is update in every call, this time is being also being use for the isKeyExpired comparison, we in case of a nested call, we want to update the cache time only in the first call (execCommand)

madolson

Thanks for this change! We should add tests for these cases as well.

src/multi.c

src/server.c

src/multi.c

oranagra · 2021-07-05T08:45:46Z

@perryitay please add a test, it can basically be formed slightly similar to the WATCH will consider touched expired keys test, but use r debug set-active-expire 0 at startup, and after 2 instead of the call for wait_for_dbsize.

zuiderkwast

Good, but it doesn't seem to be ready yet.

The build fails because keyIsExpired() is not exported from db.c. You need to add it to server.h.
There are not tests.

src/server.c

tests/unit/multi.tcl

oranagra · 2021-07-08T07:19:13Z

@redis/core-team this is a behavior change, but it actually fixes an unpredictable (timing sensitive) behavior, so i think there's no doubt about merging it (people can't really rely on this behavior).
i also think we probably wanna backport it to both 6.2 and 6.0 (where #7920 was fixed), so let me know if you think otherwise.

Co-authored-by: Oran Agra <[email protected]>

oranagra · 2021-07-08T11:06:24Z

@zuiderkwast what is there to document here?

zuiderkwast · 2021-07-08T11:06:37Z

Doc: One more note under NOTE here: https://redis.io/topics/transactions#a-hrefcommandswatchwatcha-explained

(I see now that the bullet list is messed up too. Probably a missing blank line before the bullet list.)

zuiderkwast · 2021-07-08T11:08:51Z

See redis/redis-doc#1590 (comment)

oranagra · 2021-07-08T12:02:56Z

well, i'm not sure we need to document bug history in redis.io (we have command and argument history, it may be enough).
for commands, the history is needed since you may see a certain command or argument in the docs, but it doesn't yet exists in the version you're gonna use.

bugs are documented in the release notes.. new code that's written against a fixed version will never see the bug,
and for old code that relies on the bug, you'll see the behavior change in the release notes and then you'll consider if you need to change your app code.

i also wanted to argue that in #7920 the behavior was more consistent (app can depend on it) than the one fixed here, but actually i'm not sure that's right.

anyway, bottom line, i'm ok to document it, although i'm not sure it's necessary, and if we do so, i'd advise to move that note to the bottom into some distinct history section.

soloestoy · 2021-07-08T12:20:32Z

Good job, this PR solve most of the problems(keys expired during WATCH to EXEC), but there is still a problem it doesn't solve(keys expired before WATCH but not deleted) IIUC:

Time 1: we have a key "foo" is expired but not deleted.

Time 2: we WATCH the key "foo".

Time 3: execute EXEC and we find "foo" is expired and discard the transaction, but it's not right, cause key "foo" is expired before WATCH.

soloestoy · 2021-07-08T12:22:58Z

Maybe we should call lookupKeyRead in WATCH command to trigger expired key deletion.

madolson · 2021-07-08T15:19:07Z

src/multi.c


+    /* EXEC with expired watched key is disallowed*/
+    if (isWatchedKeyExpired(c)) {
+        c->flags |= (CLIENT_DIRTY_CAS);


minor nit: The () is unnecessary and adds no value.

zuiderkwast

@soloestoy you're right! Expired but not deleted before WATCH is broken by this PR. I wrote this test case and checked that it passes on unstable but fails in this PR branch:

    test {WATCH deletes already expired keys} {
        r del x
        r debug set-active-expire 0
        r set x foo px 1
        after 2
        r watch x
        r multi
        r ping
        assert_equal {PONG} [r exec]
        r debug set-active-expire 1
    }

It needs to be fixed. This test case can be added to tests/unit/multi.tcl.

oranagra · 2021-07-09T15:06:43Z

@soloestoy @zuiderkwast i'm not sure i agree that this is really a problem (although i don't mind fixing it either).
I would still argue that the above TCL example was producing wrong results before this PR.

let's consider a more realistic examples (with similar timing as the one above):

SET x y px 1
after 2
WATCH x
GET x
MULTI
INCR x
EXEC

in this case, GET will fail the transaction.

The problem this PR fixes is the case INCR will delete the key and create a new (non-volatile) one (which is a violation of the transaction guarantees).

In the example without a GET or INCR, if the new code (this PR) will fail the transaction, there's really no harm (not a violation of the transaction guarantees).
And if it had an INCR inside it instead of PING (still without the GET between WATCH and EXEC), then this PR still fixes a real issue (watched key is deleted by one of the transaction commands).

madolson · 2021-07-09T15:21:38Z

I agree with @oranagra that I don't believe it's a real problem. (and I also don't mind fixing it either) I think it can be fixed independently though.

soloestoy · 2021-07-09T15:30:26Z

I have different opinions, I believe this is a bug, and I believe if we are sure the expire mechanism can affect transaction then we should handle all the scenarios not only the real expired deletion but also the logic expired time.

madolson · 2021-07-09T19:42:46Z

@soloestoy after thinking this through more, I suppose I do agree this is a real issue and should be addressed. Watch should call lookup key. I would still be fine merging this than fixing it though, but we should definitely fix it.

oranagra · 2021-07-11T04:59:02Z

Ok. but for clarity, let's handle it in a separate PR (different scenario, and no shared code)
@perryitay do you wanna make that PR?

itamarhaber

LGTM, haven't CR

yossigo · 2021-07-11T10:13:17Z

I also agree with @madolson and @soloestoy, WATCH should apply to the logical state and deterministic state of the key.

oranagra · 2021-07-11T10:14:45Z

ok. @perryitay will make another PR to fix the other issue.
p.s. no one commented on backporting.. i'll keep the tags and we'll discuss it when time comes..

There are two issues fixed in this commit: 1. we want to fail the EXEC command in case there is a watched key that's logically expired but not yet deleted by active expire or lazy expire. 2. we saw that currently cache time is update in every `call()` (including nested calls), this time is being also being use for the isKeyExpired comparison, we want to update the cache time only in the first call (execCommand) Co-authored-by: Oran Agra <[email protected]> (cherry picked from commit ac8b1df)

There are two issues fixed in this commit: 1. we want to fail the EXEC command in case there is a watched key that's logically expired but not yet deleted by active expire or lazy expire. 2. we saw that currently cache time is update in every `call()` (including nested calls), this time is being also being use for the isKeyExpired comparison, we want to update the cache time only in the first call (execCommand) Co-authored-by: Oran Agra <[email protected]>

After a discussion in redis#9068 and redis#9194, we reached an agreement that we should handle all scenarios about expire, not only the real expired deletion but also the logical expired time. This PR aims to fix the issue below: Time 1: we have a key "foo" is expired but not deleted. Time 2: we WATCH the key "foo". Time 3: execute EXEC and we find "foo" is expired and discard the transaction, but it's not right, cause key "foo" is expired before WATCH. To adddress the issue, the WATCH command now calls expireIfNeeded() try to delete the expired keys. But there are two scenarios that expireIfNeeded() cannot work: clients are paused and role is replica. To handle the stale key, we add a flag to record if the watchedKey is stale, and don't flag client as CLIENT_DIRTY_CAS if key is stale when touch the key.

zuiderkwast requested a review from oranagra July 4, 2021 13:04

madolson reviewed Jul 4, 2021

View reviewed changes

oranagra linked an issue Jul 5, 2021 that may be closed by this pull request

[BUG?] EXEC commands succeeds when one of the queued commands uses a WATCHed expired key #9068

Closed

Fail Exec command in case a watched key is expired

ca482a1

zuiderkwast reviewed Jul 5, 2021

View reviewed changes

perryitay force-pushed the fix-issue-9068 branch from ed875cd to ca482a1 Compare July 5, 2021 09:07

oranagra reviewed Jul 5, 2021

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

tests/unit/multi.tcl Outdated Show resolved Hide resolved

tests/unit/multi.tcl Outdated Show resolved Hide resolved

add test and fix comments

7cd3f4f

perryitay force-pushed the fix-issue-9068 branch from 0092772 to 7cd3f4f Compare July 6, 2021 08:15

oranagra reviewed Jul 6, 2021

View reviewed changes

tests/unit/multi.tcl Outdated Show resolved Hide resolved

fix the test result expectation

0ba1f91

oranagra reviewed Jul 8, 2021

View reviewed changes

tests/unit/multi.tcl Outdated Show resolved Hide resolved

oranagra previously approved these changes Jul 8, 2021

View reviewed changes

perryitay dismissed oranagra’s stale review via 0ba1f91 July 8, 2021 07:52

perryitay force-pushed the fix-issue-9068 branch from da12852 to 0ba1f91 Compare July 8, 2021 07:52

Update tests/unit/multi.tcl

f023e92

Co-authored-by: Oran Agra <[email protected]>

oranagra approved these changes Jul 8, 2021

View reviewed changes

zuiderkwast added the state:needs-doc-pr requires a PR to redis-doc repository label Jul 8, 2021

zuiderkwast mentioned this pull request Jul 8, 2021

Improve wording around expire + transact redis/redis-doc#1590

Merged

madolson approved these changes Jul 8, 2021

View reviewed changes

zuiderkwast suggested changes Jul 9, 2021

View reviewed changes

itamarhaber approved these changes Jul 11, 2021

View reviewed changes

yossigo approved these changes Jul 11, 2021

View reviewed changes

oranagra merged commit ac8b1df into redis:unstable Jul 11, 2021

soloestoy mentioned this pull request Jul 14, 2021

[WIP] WATCH command can delete expired keys #9234

Closed

oranagra added the breaking-change This change can potentially break existing application label Jul 20, 2021

This was referenced Jul 21, 2021

Release 6.2.5 #9264

Merged

Release 6.0.15 #9266

Merged

oranagra mentioned this pull request Jan 26, 2022

Should time pass in scripts? #10182

Closed

filipecosta90 mentioned this pull request Feb 24, 2022

[BUG] ZREVRANGE 50% slower after upgrading from 5.0.7 to 6.2.6 #10310

Closed

filipecosta90 mentioned this pull request Mar 21, 2022

5-7% Performance regression from v5 to v6.2 to unstable due to added features ( more visible on pipeline ) #10460

Open

oranagra mentioned this pull request Mar 22, 2022

Getex extra options to avoid duplicate options #10419

Closed

sundb mentioned this pull request Jan 13, 2023

[BUG] #11704

Open

Conversation

perryitay commented Jul 4, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jul 5, 2021

Uh oh!

zuiderkwast left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jul 8, 2021

Uh oh!

oranagra commented Jul 8, 2021

Uh oh!

zuiderkwast commented Jul 8, 2021

Uh oh!

zuiderkwast commented Jul 8, 2021

Uh oh!

oranagra commented Jul 8, 2021

Uh oh!

soloestoy commented Jul 8, 2021

Uh oh!

soloestoy commented Jul 8, 2021

Uh oh!

madolson Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

zuiderkwast left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oranagra commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson commented Jul 9, 2021

Uh oh!

soloestoy commented Jul 9, 2021

Uh oh!

madolson commented Jul 9, 2021

Uh oh!

oranagra commented Jul 11, 2021

Uh oh!

itamarhaber left a comment

Choose a reason for hiding this comment

Uh oh!

yossigo commented Jul 11, 2021

Uh oh!

oranagra commented Jul 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

perryitay commented Jul 4, 2021 •

edited by oranagra

Loading

zuiderkwast left a comment •

edited

Loading

zuiderkwast left a comment •

edited

Loading

oranagra commented Jul 9, 2021 •

edited

Loading