From d709f2a911a7f234c8b1b978b80d912300fff3cb Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Tue, 12 Mar 2024 13:50:08 +0200 Subject: [PATCH] chore: improve compatibility of EXPIRE functions with Redis (#2696) * chore: improve compatibility of EXPIRE functions with Redis Also, provide a module name if stumbled upon module data that can not be loaded by dragonfly. --------- Signed-off-by: Roman Gershman --- src/server/common.h | 3 ++- src/server/generic_family.cc | 18 +++++++++++++----- src/server/rdb_load.cc | 25 ++++++++++++++++++++++++- src/server/string_family.cc | 31 +++++++++++++++++++------------ src/server/string_family_test.cc | 3 ++- 5 files changed, 60 insertions(+), 20 deletions(-) diff --git a/src/server/common.h b/src/server/common.h index b3b5b3b72d0e..15b7fc609f80 100644 --- a/src/server/common.h +++ b/src/server/common.h @@ -23,7 +23,8 @@ namespace dfly { enum class ListDir : uint8_t { LEFT, RIGHT }; // Dependent on ExpirePeriod representation of the value. -constexpr int64_t kMaxExpireDeadlineSec = (1u << 28) - 1; +constexpr int64_t kMaxExpireDeadlineSec = (1u << 28) - 1; // 8.5 years +constexpr int64_t kMaxExpireDeadlineMs = kMaxExpireDeadlineSec * 1000; using DbIndex = uint16_t; using ShardId = uint16_t; diff --git a/src/server/generic_family.cc b/src/server/generic_family.cc index 2a970c8e8286..8c375f4001a3 100644 --- a/src/server/generic_family.cc +++ b/src/server/generic_family.cc @@ -178,7 +178,7 @@ class RestoreArgs { [[nodiscard]] bool RestoreArgs::UpdateExpiration(int64_t now_msec) { if (HasExpiration()) { int64_t ttl = abs_time_ ? expiration_ - now_msec : expiration_; - if (ttl > kMaxExpireDeadlineSec * 1000) + if (ttl > kMaxExpireDeadlineMs) return false; expiration_ = ttl < 0 ? -1 : ttl + now_msec; @@ -743,11 +743,13 @@ void GenericFamily::Expire(CmdArgList args, ConnectionContext* cntx) { return cntx->SendError(kInvalidIntErr); } - if (int_arg > kMaxExpireDeadlineSec || int_arg < -kMaxExpireDeadlineSec) { - return cntx->SendError(InvalidExpireTime(cntx->cid->name())); + int_arg = std::max(int_arg, -1); + + // silently cap the expire time to kMaxExpireDeadlineSec which is more than 8 years. + if (int_arg > kMaxExpireDeadlineSec) { + int_arg = kMaxExpireDeadlineSec; } - int_arg = std::max(int_arg, -1); auto expire_options = ParseExpireOptionsOrReply(args.subspan(2), cntx); if (!expire_options) { return; @@ -851,7 +853,13 @@ void GenericFamily::Pexpire(CmdArgList args, ConnectionContext* cntx) { if (!absl::SimpleAtoi(msec, &int_arg)) { return cntx->SendError(kInvalidIntErr); } - int_arg = std::max(int_arg, 0L); + int_arg = std::max(int_arg, -1); + + // to be more compatible with redis, we silently cap the expire time to kMaxExpireDeadlineSec + if (int_arg > kMaxExpireDeadlineMs) { + int_arg = kMaxExpireDeadlineMs; + } + auto expire_options = ParseExpireOptionsOrReply(args.subspan(2), cntx); if (!expire_options) { return; diff --git a/src/server/rdb_load.cc b/src/server/rdb_load.cc index b61772dcdf67..9c3fd1136f24 100644 --- a/src/server/rdb_load.cc +++ b/src/server/rdb_load.cc @@ -203,6 +203,25 @@ int ziplistPairsConvertAndValidateIntegrity(const uint8_t* zl, size_t size, unsi return ret; } +string ModuleTypeName(uint64_t module_id) { + static const char ModuleNameSet[] = + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "abcdefghijklmnopqrstuvwxyz" + "0123456789-_"; + + char name[10]; + + name[9] = '\0'; + char* p = name + 8; + module_id >>= 10; + for (int j = 0; j < 9; j++) { + *p-- = ModuleNameSet[module_id & 63]; + module_id >>= 6; + } + + return string{name}; +} + } // namespace class DecompressImpl { @@ -1978,7 +1997,11 @@ error_code RdbLoader::Load(io::Source* src) { } if (type == RDB_OPCODE_MODULE_AUX) { - LOG(ERROR) << "Modules are not supported"; + uint64_t module_id; + SET_OR_RETURN(LoadLen(nullptr), module_id); + string module_name = ModuleTypeName(module_id); + + LOG(ERROR) << "Modules are not supported, error loading module " << module_name; return RdbError(errc::feature_not_supported); } diff --git a/src/server/string_family.cc b/src/server/string_family.cc index 4c80d2825926..67d85c744ae6 100644 --- a/src/server/string_family.cc +++ b/src/server/string_family.cc @@ -739,16 +739,16 @@ void StringFamily::Set(CmdArgList args, ConnectionContext* cntx) { return builder->SendStored(); } } - if (!is_ms && int_arg >= kMaxExpireDeadlineSec) { - return builder->SendError(InvalidExpireTime("set")); - } - - if (!is_ms) { + if (is_ms) { + if (int_arg > kMaxExpireDeadlineMs) { + int_arg = kMaxExpireDeadlineMs; + } + } else { + if (int_arg > kMaxExpireDeadlineSec) { + int_arg = kMaxExpireDeadlineSec; + } int_arg *= 1000; } - if (int_arg >= kMaxExpireDeadlineSec * 1000) { - return builder->SendError(InvalidExpireTime("set")); - } sparams.expire_after_ms = int_arg; } else if (cur_arg == "NX" && !(sparams.flags & SetCmd::SET_IF_EXISTS)) { sparams.flags |= SetCmd::SET_IF_NOTEXIST; @@ -1147,22 +1147,29 @@ void StringFamily::SetExGeneric(bool seconds, CmdArgList args, ConnectionContext string_view key = ArgS(args, 0); string_view ex = ArgS(args, 1); string_view value = ArgS(args, 2); - int32_t unit_vals; + int64_t unit_vals; if (!absl::SimpleAtoi(ex, &unit_vals)) { return cntx->SendError(kInvalidIntErr); } - if (unit_vals < 1 || unit_vals >= kMaxExpireDeadlineSec) { + if (unit_vals < 1) { return cntx->SendError(InvalidExpireTime(cntx->cid->name())); } SetCmd::SetParams sparams; sparams.flags |= SetCmd::SET_EXPIRE_AFTER_MS; - if (seconds) + if (seconds) { + if (unit_vals > kMaxExpireDeadlineSec) { + unit_vals = kMaxExpireDeadlineSec; + } sparams.expire_after_ms = uint64_t(unit_vals) * 1000; - else + } else { + if (unit_vals > kMaxExpireDeadlineMs) { + unit_vals = kMaxExpireDeadlineMs; + } sparams.expire_after_ms = unit_vals; + } auto cb = [&](Transaction* t, EngineShard* shard) { SetCmd sg(t->GetOpArgs(shard), true); diff --git a/src/server/string_family_test.cc b/src/server/string_family_test.cc index 291a2c96d422..fe1359d2393d 100644 --- a/src/server/string_family_test.cc +++ b/src/server/string_family_test.cc @@ -451,7 +451,8 @@ TEST_F(StringFamilyTest, SetEx) { ASSERT_THAT(Run({"ttl", "key"}), IntArg(10)); ASSERT_THAT(Run({"setex", "key", "0", "val"}), ErrArg("invalid expire time")); ASSERT_EQ(Run({"setex", "key", StrCat(5 * 365 * 24 * 3600), "val"}), "OK"); - ASSERT_THAT(Run({"setex", "key", StrCat(1 << 30), "val"}), ErrArg("invalid expire time")); + ASSERT_THAT(Run({"setex", "key", StrCat(1 << 30), "val"}), "OK"); + ASSERT_THAT(Run({"ttl", "key"}), IntArg(kMaxExpireDeadlineSec)); } TEST_F(StringFamilyTest, Range) {