From c98bc934f2c39135cb0e8c65c608921d16d5a758 Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Mon, 28 Mar 2022 17:02:03 +0300 Subject: [PATCH] Fix (P)TTL issue with non-existing keys. Improve logging for binary values. Fix RENAME bugs --- src/facade/facade.cc | 9 +++-- src/server/generic_family.cc | 61 ++++++++++++++++++------------- src/server/generic_family_test.cc | 20 +++++++++- src/server/main_service.cc | 2 +- 4 files changed, 61 insertions(+), 31 deletions(-) diff --git a/src/facade/facade.cc b/src/facade/facade.cc index bd3cc80..aec4c02 100644 --- a/src/facade/facade.cc +++ b/src/facade/facade.cc @@ -2,10 +2,10 @@ // See LICENSE for licensing terms. // +#include #include #include "base/logging.h" - #include "facade/conn_context.h" #include "facade/dragonfly_connection.h" #include "facade/error.h" @@ -112,16 +112,17 @@ RedisReplyBuilder* ConnectionContext::operator->() { } // namespace facade - namespace std { +using facade::ArgS; + ostream& operator<<(ostream& os, facade::CmdArgList ras) { os << "["; if (!ras.empty()) { for (size_t i = 0; i < ras.size() - 1; ++i) { - os << facade::ArgS(ras, i) << ","; + os << absl::CHexEscape(ArgS(ras, i)) << ","; } - os << facade::ArgS(ras, ras.size() - 1); + os << absl::CHexEscape(ArgS(ras, ras.size() - 1)); } os << "]"; diff --git a/src/server/generic_family.cc b/src/server/generic_family.cc index 507162d..379a7a0 100644 --- a/src/server/generic_family.cc +++ b/src/server/generic_family.cc @@ -321,14 +321,18 @@ void GenericFamily::TtlGeneric(CmdArgList args, ConnectionContext* cntx, TimeUni if (result) { long ttl = (unit == TimeUnit::SEC) ? (result.value() + 500) / 1000 : result.value(); (*cntx)->SendLong(ttl); - } else { - switch (result.status()) { - case OpStatus::KEY_NOTFOUND: - (*cntx)->SendLong(-1); - break; - default: - (*cntx)->SendLong(-2); - } + return; + } + + switch (result.status()) { + case OpStatus::KEY_NOTFOUND: + (*cntx)->SendLong(-2); + break; + default: + LOG_IF(ERROR, result.status() != OpStatus::SKIPPED) + << "Unexpected status " << result.status(); + (*cntx)->SendLong(-1); + break; } } @@ -528,7 +532,7 @@ OpResult GenericFamily::OpExists(const OpArgs& op_args, ArgSlice keys) OpResult GenericFamily::OpRen(const OpArgs& op_args, string_view from, string_view to, bool skip_exists) { auto& db_slice = op_args.shard->db_slice(); - auto [from_it, expire_it] = db_slice.FindExt(op_args.db_ind, from); + auto [from_it, from_expire] = db_slice.FindExt(op_args.db_ind, from); if (!IsValid(from_it)) return OpStatus::KEY_NOTFOUND; @@ -538,25 +542,32 @@ OpResult GenericFamily::OpRen(const OpArgs& op_args, string_view from, str return OpStatus::KEY_EXISTS; } - uint64_t exp_ts = IsValid(expire_it) ? expire_it->second.duration() : 0; - if (IsValid(to_it)) { - to_it->second = std::move(from_it->second); - from_it->second.SetExpire(IsValid(expire_it)); + uint64_t exp_ts = + IsValid(from_expire) ? db_slice.expire_base() + from_expire->second.duration() : 0; - if (IsValid(to_expire)) { - to_it->second.SetExpire(true); - to_expire->second.Set(exp_ts); - } else { - to_it->second.SetExpire(false); - db_slice.Expire(op_args.db_ind, to_it, exp_ts + db_slice.expire_base()); - } + // we keep the value we want to move. + PrimeValue from_obj = std::move(from_it->second); + + // Restore the expire flag on 'from'. + from_it->second.SetExpire(IsValid(from_expire)); + + if (IsValid(to_it)) { + to_it->second = std::move(from_obj); + to_it->second.SetExpire(IsValid(to_expire)); // keep the expire flag on 'to'. + + // It is guaranteed that Expire() call does not erase the element because then + // from_it would be invalid. Therefore, it Expire does not invalidate any iterators and + // we can delete via from_it. + db_slice.Expire(op_args.db_ind, to_it, exp_ts); + CHECK(db_slice.Del(op_args.db_ind, from_it)); } else { - db_slice.AddNew(op_args.db_ind, to, std::move(from_it->second), - exp_ts + db_slice.expire_base()); - // Need search again since the container might invalidate the iterators. - from_it = db_slice.FindExt(op_args.db_ind, from).first; + // Here we first delete from_it because AddNew below could invalidate from_it. + // On the other hand, AddNew does not rely on the iterators - this is why we keep + // the value in `from_obj`. + CHECK(db_slice.Del(op_args.db_ind, from_it)); + db_slice.AddNew(op_args.db_ind, to, std::move(from_obj), exp_ts); } - CHECK(db_slice.Del(op_args.db_ind, from_it)); + return OpStatus::OK; } diff --git a/src/server/generic_family_test.cc b/src/server/generic_family_test.cc index 213d32d..9a961ba 100644 --- a/src/server/generic_family_test.cc +++ b/src/server/generic_family_test.cc @@ -1,4 +1,4 @@ -// Copyright 2021, Roman Gershman. All rights reserved. +// Copyright 2022, Roman Gershman. All rights reserved. // See LICENSE for licensing terms. // @@ -79,6 +79,14 @@ TEST_F(GenericFamilyTest, Del) { del_fb.join(); } +TEST_F(GenericFamilyTest, TTL) { + EXPECT_EQ(-2, CheckedInt({"ttl", "foo"})); + EXPECT_EQ(-2, CheckedInt({"pttl", "foo"})); + Run({"set", "foo", "bar"}); + EXPECT_EQ(-1, CheckedInt({"ttl", "foo"})); + EXPECT_EQ(-1, CheckedInt({"pttl", "foo"})); +} + TEST_F(GenericFamilyTest, Exists) { Run({"mset", "x", "0", "y", "1"}); auto resp = Run({"exists", "x", "y", "x"}); @@ -128,4 +136,14 @@ TEST_F(GenericFamilyTest, Rename) { } +TEST_F(GenericFamilyTest, RenameBinary) { + const char kKey1[] = "\x01\x02\x03\x04"; + const char kKey2[] = "\x05\x06\x07\x08"; + + Run({"set", kKey1, "bar"}); + Run({"rename", kKey1, kKey2}); + EXPECT_THAT(Run({"get", kKey1}), ElementsAre(ArgType(RespExpr::NIL))); + EXPECT_THAT(Run({"get", kKey2}), RespEq("bar")); +} + } // namespace dfly diff --git a/src/server/main_service.cc b/src/server/main_service.cc index 9b0dcc2..9ffc3fa 100644 --- a/src/server/main_service.cc +++ b/src/server/main_service.cc @@ -393,7 +393,7 @@ void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx) string_view cmd_name{cid->name()}; if (cntx->req_auth && !cntx->authenticated) { - if (cmd_name != "AUTH") { + if (cmd_name != "AUTH" && cmd_name != "QUIT") { return (*cntx)->SendError("-NOAUTH Authentication required."); } }