Skip to content

Commit

Permalink
[Bug] Fix for SET EX NX does not set a string longer than expired (#772)
Browse files Browse the repository at this point in the history 
* Fix for SET EX NX does not set a string longer than expired

* PR review

* Review comments

---------

Co-authored-by: Badrish Chandramouli <[email protected]>
  • Loading branch information
@Vijay-Nirmal @badrishc
Vijay-Nirmal and badrishc authored Nov 8, 2024
1 parent f95a203 commit 9862534
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 7 deletions.
2 changes: 1 addition & 1 deletion libs/server/Resp/BasicCommands.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@ private bool NetworkSET_Conditional<TGarnetApi>(RespCommand cmd, int expiry, ref
if (status == GarnetStatus.NOTFOUND)
{
Debug.Assert(o.IsSpanByte);
while (!RespWriteUtils.WriteDirect(CmdStrings.RESP_ERRNOTFOUND, ref dcurr, dend))
while (!RespWriteUtils.WriteNull(ref dcurr, dend))
SendAndReset();
}
else
Expand Down
13 changes: 11 additions & 2 deletions libs/server/Storage/Functions/MainStore/RMWMethods.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -560,10 +560,10 @@ public bool NeedCopyUpdate(ref SpanByte key, ref RawStringInput input, ref SpanB
{
case RespCommand.SETEXNX:
// Expired data, return false immediately
// ExpireAndStop ensures that caller sees a NOTFOUND status
// ExpireAndResume ensures that we set as new value, since it does not exist
if (oldValue.MetadataSize > 0 && input.header.CheckExpiry(oldValue.ExtraMetadata))
{
rmwInfo.Action = RMWAction.ExpireAndStop;
rmwInfo.Action = RMWAction.ExpireAndResume;
return false;
}
// Check if SetGet flag is set
Expand All @@ -573,6 +573,15 @@ public bool NeedCopyUpdate(ref SpanByte key, ref RawStringInput input, ref SpanB
CopyRespTo(ref oldValue, ref output);
}
return false;
case RespCommand.SETEXXX:
// Expired data, return false immediately so we do not set, since it does not exist
// ExpireAndStop ensures that caller sees a NOTFOUND status
if (oldValue.MetadataSize > 0 && input.header.CheckExpiry(oldValue.ExtraMetadata))
{
rmwInfo.Action = RMWAction.ExpireAndStop;
return false;
}
return true;
default:
if ((ushort)input.header.cmd >= CustomCommandManager.StartOffset)
{
Expand Down
112 changes: 108 additions & 4 deletions test/Garnet.test/RespTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,15 +323,75 @@ public void SetExpiryNx()
using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
var db = redis.GetDatabase(0);

var key = "mykey";
string origValue = "abcdefghij";
db.StringSet("mykey", origValue, TimeSpan.FromSeconds(3), When.NotExists, CommandFlags.None);
var newLongerValue = "abcdefghijk"; // Longer value to test non in-place update
var newShorterValue = "abcdefghi"; // Shorter value to test in-place update
var expireTime = TimeSpan.FromSeconds(1);
var ok = db.StringSet(key, origValue, expireTime, When.NotExists, CommandFlags.None);
ClassicAssert.IsTrue(ok);

string retValue = db.StringGet("mykey");
string retValue = db.StringGet(key);
ClassicAssert.AreEqual(origValue, retValue);

Thread.Sleep(4000);
retValue = db.StringGet("mykey");
Thread.Sleep(expireTime * 1.1);
retValue = db.StringGet(key);
ClassicAssert.AreEqual(null, retValue);

// Test non in-place update
ok = db.StringSet(key, newLongerValue, expireTime, When.NotExists, CommandFlags.None);
ClassicAssert.IsTrue(ok);

retValue = db.StringGet(key);
ClassicAssert.AreEqual(newLongerValue, retValue);

Thread.Sleep(expireTime * 1.1);

// Test in-place update
ok = db.StringSet(key, newShorterValue, expireTime, When.NotExists, CommandFlags.None);
ClassicAssert.IsTrue(ok);

retValue = db.StringGet(key);
ClassicAssert.AreEqual(newShorterValue, retValue);
}

[Test]
public void SetAndGetExpiryNx()
{
using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
var db = redis.GetDatabase(0);

var key = "mykey";
string origValue = "abcdefghij";
var newValue = "xyz"; // New value that shouldn't be written
var newLongerValue = "abcdefghijk"; // Longer value to test non in-place update
var newShorterValue = "abcdefghi"; // Shorter value to test in-place update
var expireTime = TimeSpan.FromSeconds(1);
string actualValue = db.StringSetAndGet(key, origValue, expireTime, when: When.NotExists, CommandFlags.None);
ClassicAssert.IsNull(actualValue);

string retValue = db.StringSetAndGet(key, newValue, expireTime, when: When.NotExists, CommandFlags.None);
ClassicAssert.AreEqual(origValue, retValue);

Thread.Sleep(expireTime * 1.1);
retValue = db.StringGet(key);
ClassicAssert.AreEqual(null, retValue);

// Test non in-place update
actualValue = db.StringSetAndGet(key, newLongerValue, expireTime, when: When.NotExists, CommandFlags.None);
ClassicAssert.IsNull(actualValue);

retValue = db.StringGet(key);
ClassicAssert.AreEqual(newLongerValue, retValue);

Thread.Sleep(expireTime * 1.1);

// Test in-place update
actualValue = db.StringSetAndGet(key, newShorterValue, expireTime, when: When.NotExists, CommandFlags.None);
ClassicAssert.IsNull(actualValue);

retValue = db.StringGet(key);
ClassicAssert.AreEqual(newShorterValue, retValue);
}

[Test]
Expand Down Expand Up @@ -364,6 +424,50 @@ public void SetXx()
ClassicAssert.AreEqual(newValue, retValue);
}

[Test]
public void SetAndGetExpiryXx()
{
using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
var db = redis.GetDatabase(0);

var key = "mykey";
string origValue = "abcdefghij";
var newValue = "xyz"; // New value that shouldn't be written
var newLongerValue = "abcdefghijk"; // Longer value to test non in-place update
var newShorterValue = "abcdefghi"; // Shorter value to test in-place update
var expireTime = TimeSpan.FromSeconds(1);
string actualValue = db.StringSetAndGet(key, newValue, expireTime, when: When.Exists, CommandFlags.None);
ClassicAssert.IsNull(actualValue);

string retValue = db.StringGet(key);
ClassicAssert.IsNull(retValue);

db.StringSet(key, origValue);
// Test non in-place update
actualValue = db.StringSetAndGet(key, newLongerValue, expireTime, when: When.Exists, CommandFlags.None);
ClassicAssert.AreEqual(origValue, actualValue);

retValue = db.StringGet(key);
ClassicAssert.AreEqual(newLongerValue, retValue);

Thread.Sleep(expireTime * 1.1);
retValue = db.StringGet(key);
ClassicAssert.IsNull(retValue);


db.StringSet(key, origValue);
// Test in-place update
actualValue = db.StringSetAndGet(key, newShorterValue, expireTime, when: When.Exists, CommandFlags.None);
ClassicAssert.AreEqual(origValue, actualValue);

retValue = db.StringGet(key);
ClassicAssert.AreEqual(newShorterValue, retValue);

Thread.Sleep(expireTime * 1.1);
retValue = db.StringGet(key);
ClassicAssert.IsNull(retValue);
}

[Test]
public void SetGet()
{
Expand Down

0 comments on commit 9862534

Please sign in to comment.