From 87e6d357e98b2397324e20321b1292b9fdf8fd90 Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 15 Apr 2020 23:21:10 -0400 Subject: [PATCH 01/24] Update Readme to use github CI badge Former-commit-id: 7cdd5d9e4108474edb453e4be98a1e95f01400d1 --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 14b98a421..06d85c86e 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,6 @@ ![Current Release](https://img.shields.io/github/release/JohnSully/KeyDB.svg) -[![Build Status](https://travis-ci.org/JohnSully/KeyDB.svg?branch=unstable)](https://travis-ci.org/JohnSully/KeyDB) [![Join the chat at https://gitter.im/KeyDB/community](https://badges.gitter.im/KeyDB/community.svg)](https://gitter.im/KeyDB/community?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge) +![CI](https://github.com/JohnSully/KeyDB/workflows/CI/badge.svg?branch=unstable) +[![Join the chat at https://gitter.im/KeyDB/community](https://badges.gitter.im/KeyDB/community.svg)](https://gitter.im/KeyDB/community?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge) [![StackShare](http://img.shields.io/badge/tech-stack-0690fa.svg?style=flat)](https://stackshare.io/eq-alpha-technology-inc/eq-alpha-technology-inc) ##### Need Help? Check out our extensive [documentation](https://docs.keydb.dev). From e5a1f845823c5830a5a91d9c074d3a0ec1ff77b9 Mon Sep 17 00:00:00 2001 From: John Sully Date: Thu, 16 Apr 2020 20:01:13 -0400 Subject: [PATCH 02/24] Fix warning in cluster.cpp Former-commit-id: e5e33f8cb7b4c928884a934184cbf81b916b3c03 --- src/cluster.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cluster.cpp b/src/cluster.cpp index 181516437..aa366c6ef 100644 --- a/src/cluster.cpp +++ b/src/cluster.cpp @@ -4246,7 +4246,7 @@ void clusterReplyMultiBulkSlots(client *c) { static_assert((CLUSTER_SLOTS % (sizeof(uint32_t)*8)) == 0, "code below assumes the bitfield is a multiple of sizeof(unsinged)"); - for (unsigned iw = 0; iw < (CLUSTER_SLOTS/sizeof(uint32_t)/8); ++iw) + for (int iw = 0; iw < (CLUSTER_SLOTS/(int)sizeof(uint32_t)/8); ++iw) { uint32_t wordCur = reinterpret_cast(node->slots)[iw]; if (iw != ((CLUSTER_SLOTS/sizeof(uint32_t)/8)-1)) @@ -4259,7 +4259,7 @@ void clusterReplyMultiBulkSlots(client *c) { unsigned ibitStartLoop = iw*sizeof(uint32_t)*8; - for (unsigned j = ibitStartLoop; j < (iw+1)*sizeof(uint32_t)*8; j++) { + for (int j = ibitStartLoop; j < (iw+1)*(int)sizeof(uint32_t)*8; j++) { int i; int bit = (int)(wordCur & 1); wordCur >>= 1; From 3ecb621ccfe6c169169375a22ca8f14d61bb5db2 Mon Sep 17 00:00:00 2001 From: John Sully Date: Thu, 16 Apr 2020 20:01:24 -0400 Subject: [PATCH 03/24] Always link libatomic Former-commit-id: 7aeae62f84fe958bcda9925f76180a7e149a337e --- src/Makefile | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/Makefile b/src/Makefile index 78568b736..197e5beea 100644 --- a/src/Makefile +++ b/src/Makefile @@ -83,10 +83,6 @@ ifneq (,$(findstring armv,$(uname_M))) endif endif -ifneq (,$(findstring armv,$(uname_M))) - FINAL_LIBS+=-latomic -endif - # Backwards compatibility for selecting an allocator ifeq ($(USE_TCMALLOC),yes) MALLOC=tcmalloc @@ -110,18 +106,9 @@ endif FINAL_CFLAGS=$(STD) $(WARN) $(OPT) $(DEBUG) $(CFLAGS) $(REDIS_CFLAGS) FINAL_CXXFLAGS=$(CXX_STD) $(WARN) $(OPT) $(DEBUG) $(CXXFLAGS) $(REDIS_CFLAGS) FINAL_LDFLAGS=$(LDFLAGS) $(REDIS_LDFLAGS) $(DEBUG) -FINAL_LIBS+=-lm -lcurl +FINAL_LIBS+=-lm -lcurl -latomic DEBUG=-g -ggdb -# Linux ARM needs -latomic at linking time -ifneq (,$(filter aarch64 armv,$(uname_M))) - FINAL_LIBS+=-latomic -else -ifneq (,$(findstring armv,$(uname_M))) - FINAL_LIBS+=-latomic -endif -endif - ifeq ($(uname_S),SunOS) # SunOS ifneq ($(@@),32bit) From 0b9779c33f0870c27f537530d623b53b7c4a3ff5 Mon Sep 17 00:00:00 2001 From: John Sully Date: Thu, 16 Apr 2020 20:12:05 -0400 Subject: [PATCH 04/24] Fix some clang warnings Former-commit-id: 3785b1efd49002e629c1d821c57e971f8f09a2d2 --- src/fastlock.cpp | 2 -- src/fastlock.h | 2 +- src/geohash_helper.cpp | 3 --- 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/fastlock.cpp b/src/fastlock.cpp index a1cacfecd..5a7152b00 100644 --- a/src/fastlock.cpp +++ b/src/fastlock.cpp @@ -140,13 +140,11 @@ extern "C" void unlock_futex(struct fastlock *lock, uint16_t ifutex); #endif -#pragma weak _serverPanic extern "C" __attribute__((weak)) void _serverPanic(const char * /*file*/, int /*line*/, const char * /*msg*/, ...) { *((char*)-1) = 'x'; } -#pragma weak serverLog __attribute__((weak)) void serverLog(int , const char *fmt, ...) { va_list args; diff --git a/src/fastlock.h b/src/fastlock.h index 44c09ac17..cdf8fe454 100644 --- a/src/fastlock.h +++ b/src/fastlock.h @@ -84,4 +84,4 @@ struct fastlock #endif }; -static_assert(offsetof(struct fastlock, m_ticket) == 64, "ensure padding is correct"); \ No newline at end of file +static_assert(offsetof(struct fastlock, m_ticket) == 64, "ensure padding is correct"); diff --git a/src/geohash_helper.cpp b/src/geohash_helper.cpp index e23f17b4e..cf9716352 100644 --- a/src/geohash_helper.cpp +++ b/src/geohash_helper.cpp @@ -46,13 +46,10 @@ #define ECCENT (sqrt(1.0 - (RATIO *RATIO))) #define COM (0.5 * ECCENT) -/// @brief The usual PI/180 constant -const double DEG_TO_RAD = 0.017453292519943295769236907684886; /// @brief Earth's quatratic mean radius for WGS-84 const double EARTH_RADIUS_IN_METERS = 6372797.560856; const double MERCATOR_MAX = 20037726.37; -const double MERCATOR_MIN = -20037726.37; static inline double deg_rad(double ang) { return ang * D_R; } static inline double rad_deg(double ang) { return ang / D_R; } From f34e25393ed4a88a77be37b1c48e99249abb25b7 Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 22 Apr 2020 01:21:35 -0400 Subject: [PATCH 05/24] MacOS Build Break Fix Former-commit-id: d6738ffa5df6dd4d6cdf9f717d30f163d3dc0129 --- src/Makefile | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Makefile b/src/Makefile index 197e5beea..bc60b4d74 100644 --- a/src/Makefile +++ b/src/Makefile @@ -106,9 +106,14 @@ endif FINAL_CFLAGS=$(STD) $(WARN) $(OPT) $(DEBUG) $(CFLAGS) $(REDIS_CFLAGS) FINAL_CXXFLAGS=$(CXX_STD) $(WARN) $(OPT) $(DEBUG) $(CXXFLAGS) $(REDIS_CFLAGS) FINAL_LDFLAGS=$(LDFLAGS) $(REDIS_LDFLAGS) $(DEBUG) -FINAL_LIBS+=-lm -lcurl -latomic +FINAL_LIBS+=-lm -lcurl DEBUG=-g -ggdb +ifneq ($(uname_S),Darwin) + FINAL_LIBS+=-latomic +endif + + ifeq ($(uname_S),SunOS) # SunOS ifneq ($(@@),32bit) From 2909b19cdb944517f02b68d5b79dbdc485ad4f13 Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 22 Apr 2020 02:13:02 -0400 Subject: [PATCH 06/24] Run arm CI builds Former-commit-id: 17795fd5ce5fed0706769b37b3d11104b575fc37 --- .github/workflows/ci.yml | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d812760c9..8c8a47fce 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,7 +10,20 @@ jobs: - name: make run: | sudo apt-get install uuid-dev libcurl4-openssl-dev - make + make -j2 + - name: test + run: | + sudo apt-get install tcl8.5 + ./runtest --clients 2 --verbose + + test-ubuntu-arm: + runs-on: [self-hosted, linux, arm] + steps: + - uses: actions/checkout@v1 + - name: make + run: | + sudo apt-get install uuid-dev libcurl4-openssl-dev + make -j4 - name: test run: | sudo apt-get install tcl8.5 @@ -20,7 +33,7 @@ jobs: runs-on: ubuntu-16.04 steps: - uses: actions/checkout@v1 - - name: make + - name: make -j2 run: | sudo apt-get install uuid-dev libcurl4-openssl-dev make @@ -30,4 +43,4 @@ jobs: steps: - uses: actions/checkout@v1 - name: make - run: make + run: make -j2 From 3395d984eb677bd6cd4b63ef2a783de869b5b07d Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 22 Apr 2020 02:17:28 -0400 Subject: [PATCH 07/24] Default yes in apt install Former-commit-id: e0a8709c09796b4e9a32166205ebdd487b52290a --- .github/workflows/ci.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c8a47fce..61309ac21 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,11 +9,11 @@ jobs: - uses: actions/checkout@v1 - name: make run: | - sudo apt-get install uuid-dev libcurl4-openssl-dev + sudo apt-get -y install uuid-dev libcurl4-openssl-dev make -j2 - name: test run: | - sudo apt-get install tcl8.5 + sudo apt-get -y install tcl8.5 ./runtest --clients 2 --verbose test-ubuntu-arm: @@ -22,11 +22,11 @@ jobs: - uses: actions/checkout@v1 - name: make run: | - sudo apt-get install uuid-dev libcurl4-openssl-dev + sudo apt-get -y install uuid-dev libcurl4-openssl-dev make -j4 - name: test run: | - sudo apt-get install tcl8.5 + sudo apt-get -y install tcl8.5 ./runtest --clients 2 --verbose build-ubuntu-old: @@ -35,7 +35,7 @@ jobs: - uses: actions/checkout@v1 - name: make -j2 run: | - sudo apt-get install uuid-dev libcurl4-openssl-dev + sudo apt-get -y install uuid-dev libcurl4-openssl-dev make build-macos-latest: From 7f560f8e65a3d9d61b521e26c51d24390ae2e417 Mon Sep 17 00:00:00 2001 From: John Sully Date: Fri, 24 Apr 2020 22:19:55 -0400 Subject: [PATCH 08/24] EMBSTR size is lower than it needs to be Former-commit-id: fab6132cb3a0594f6ef65163fcb6f1e0ff8d7587 --- src/object.cpp | 5 +++-- src/storage.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/object.cpp b/src/object.cpp index 77fc1c732..7d07d2de4 100644 --- a/src/object.cpp +++ b/src/object.cpp @@ -127,9 +127,10 @@ robj *createEmbeddedStringObject(const char *ptr, size_t len) { * OBJ_ENCODING_EMBSTR_SIZE_LIMIT, otherwise the RAW encoding is * used. * - * The current limit of 44 is chosen so that the biggest string object + * The current limit of 52 is chosen so that the biggest string object * we allocate as EMBSTR will still fit into the 64 byte arena of jemalloc. */ -#define OBJ_ENCODING_EMBSTR_SIZE_LIMIT 44 +#define OBJ_ENCODING_EMBSTR_SIZE_LIMIT 48 +static_assert((sizeof(redisObject)+OBJ_ENCODING_EMBSTR_SIZE_LIMIT-8) == 64, "Max EMBSTR obj should be 64 bytes total"); robj *createStringObject(const char *ptr, size_t len) { if (len <= OBJ_ENCODING_EMBSTR_SIZE_LIMIT) return createEmbeddedStringObject(ptr,len); diff --git a/src/storage.h b/src/storage.h index 5aee09817..e9106aca2 100644 --- a/src/storage.h +++ b/src/storage.h @@ -1,7 +1,7 @@ #ifndef __STORAGE_H__ #define __STORAGE_H__ -#define OBJ_ENCODING_EMBSTR_SIZE_LIMIT 44 // Note: also defined in object.c - should always match +#define OBJ_ENCODING_EMBSTR_SIZE_LIMIT 48 // Note: also defined in object.c - should always match #ifdef __cplusplus extern "C" { From ad06da56551dfa4ee21f16ec7b2be17e753da2b6 Mon Sep 17 00:00:00 2001 From: John Sully Date: Fri, 24 Apr 2020 22:20:26 -0400 Subject: [PATCH 09/24] shared pointer comparisons with other pointers Former-commit-id: d5ede50b040c82e02eb2b82982091bdd0fb7da12 --- src/server.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/server.h b/src/server.h index 8870a6a07..a7a1e2adc 100644 --- a/src/server.h +++ b/src/server.h @@ -210,11 +210,21 @@ public: return m_ptr == other.m_ptr; } + bool operator==(const void *p) const + { + return m_ptr == p; + } + bool operator!=(const robj_sharedptr &other) const { return m_ptr != other.m_ptr; } + bool operator!=(const void *p) const + { + return m_ptr != p; + } + redisObject* operator->() const { return m_ptr; From a1d9c2e827aa84879132be0b7bac53105d0ea880 Mon Sep 17 00:00:00 2001 From: John Sully Date: Tue, 28 Apr 2020 22:41:07 -0400 Subject: [PATCH 10/24] use serverAssert() instead of assert() to get callstacks in fastlock Former-commit-id: 45535e8a6377963dce5b158a9a6e448c5c22a0a8 --- src/fastlock.cpp | 7 ++++--- src/server.h | 14 +------------- src/serverassert.h | 15 +++++++++++++++ 3 files changed, 20 insertions(+), 16 deletions(-) create mode 100644 src/serverassert.h diff --git a/src/fastlock.cpp b/src/fastlock.cpp index 5a7152b00..bb8621906 100644 --- a/src/fastlock.cpp +++ b/src/fastlock.cpp @@ -45,6 +45,7 @@ #include #include #include "config.h" +#include "serverassert.h" #ifdef __APPLE__ #include @@ -164,7 +165,7 @@ extern "C" pid_t gettid() if (pidCache == -1) { uint64_t tidT; pthread_threadid_np(nullptr, &tidT); - assert(tidT < UINT_MAX); + serverAssert(tidT < UINT_MAX); pidCache = (int)tidT; } #endif @@ -393,7 +394,7 @@ extern "C" void fastlock_unlock(struct fastlock *lock) { int pidT; __atomic_load(&lock->m_pidOwner, &pidT, __ATOMIC_RELAXED); - assert(pidT >= 0); // unlock after free + serverAssert(pidT >= 0); // unlock after free int t = -1; __atomic_store(&lock->m_pidOwner, &t, __ATOMIC_RELEASE); std::atomic_thread_fence(std::memory_order_release); @@ -431,7 +432,7 @@ extern "C" void unlock_futex(struct fastlock *lock, uint16_t ifutex) extern "C" void fastlock_free(struct fastlock *lock) { // NOP - assert((lock->m_ticket.m_active == lock->m_ticket.m_avail) // Asser the lock is unlocked + serverAssert((lock->m_ticket.m_active == lock->m_ticket.m_avail) // Asser the lock is unlocked || (lock->m_pidOwner == gettid() && (lock->m_ticket.m_active == lock->m_ticket.m_avail-1))); // OR we own the lock and nobody else is waiting lock->m_pidOwner = -2; // sentinal value indicating free ANNOTATE_RWLOCK_DESTROY(lock); diff --git a/src/server.h b/src/server.h index a7a1e2adc..9d13311e2 100644 --- a/src/server.h +++ b/src/server.h @@ -92,6 +92,7 @@ typedef long long ustime_t; /* microsecond time type. */ #include "uuid.h" #include "semiorderedset.h" #include "connection.h" /* Connection abstraction */ +#include "serverassert.h" #define REDISMODULE_CORE 1 #include "redismodule.h" /* Redis modules API defines. */ @@ -610,16 +611,6 @@ public: * The actual resolution depends on g_pserver->hz. */ #define run_with_period(_ms_) if ((_ms_ <= 1000/g_pserver->hz) || !(g_pserver->cronloops%((_ms_)/(1000/g_pserver->hz)))) -/* We can print the stacktrace, so our assert is defined this way: */ -#define serverAssertWithInfo(_c,_o,_e) ((_e)?(void)0 : (_serverAssertWithInfo(_c,_o,#_e,__FILE__,__LINE__),_exit(1))) -#define serverAssert(_e) ((_e)?(void)0 : (_serverAssert(#_e,__FILE__,__LINE__),_exit(1))) -#ifdef _DEBUG -#define serverAssertDebug(_e) serverAssert(_e) -#else -#define serverAssertDebug(_e) -#endif -#define serverPanic(...) _serverPanic(__FILE__,__LINE__,__VA_ARGS__),_exit(1) - /*----------------------------------------------------------------------------- * Data types *----------------------------------------------------------------------------*/ @@ -2996,9 +2987,6 @@ void *realloc(void *ptr, size_t size); #endif /* Debugging stuff */ -void _serverAssertWithInfo(const client *c, robj_roptr o, const char *estr, const char *file, int line); -extern "C" void _serverAssert(const char *estr, const char *file, int line); -extern "C" void _serverPanic(const char *file, int line, const char *msg, ...); void bugReportStart(void); void serverLogObjectDebugInfo(robj_roptr o); void sigsegvHandler(int sig, siginfo_t *info, void *secret); diff --git a/src/serverassert.h b/src/serverassert.h new file mode 100644 index 000000000..91223c2b1 --- /dev/null +++ b/src/serverassert.h @@ -0,0 +1,15 @@ +#pragma once + +void _serverAssertWithInfo(const struct client *c, class robj_roptr o, const char *estr, const char *file, int line); +extern "C" void _serverAssert(const char *estr, const char *file, int line); +extern "C" void _serverPanic(const char *file, int line, const char *msg, ...); + +/* We can print the stacktrace, so our assert is defined this way: */ +#define serverAssertWithInfo(_c,_o,_e) ((_e)?(void)0 : (_serverAssertWithInfo(_c,_o,#_e,__FILE__,__LINE__),_exit(1))) +#define serverAssert(_e) ((_e)?(void)0 : (_serverAssert(#_e,__FILE__,__LINE__),_exit(1))) +#ifdef _DEBUG +#define serverAssertDebug(_e) serverAssert(_e) +#else +#define serverAssertDebug(_e) +#endif +#define serverPanic(...) _serverPanic(__FILE__,__LINE__,__VA_ARGS__),_exit(1) \ No newline at end of file From c322823f5e0d0e4a664c121670dcc020fd0ec46a Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 29 Apr 2020 18:41:51 -0400 Subject: [PATCH 11/24] Run cluster tests as part of CI Former-commit-id: 98d690b8499d0c3085ce56021dac499349898850 --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 61309ac21..026d00592 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,6 +15,7 @@ jobs: run: | sudo apt-get -y install tcl8.5 ./runtest --clients 2 --verbose + ./runtest-cluster test-ubuntu-arm: runs-on: [self-hosted, linux, arm] @@ -28,6 +29,7 @@ jobs: run: | sudo apt-get -y install tcl8.5 ./runtest --clients 2 --verbose + ./runtest-cluster build-ubuntu-old: runs-on: ubuntu-16.04 From cf13892cfa4181608fdb1ae1ac904abb279eb11e Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 29 Apr 2020 18:43:55 -0400 Subject: [PATCH 12/24] cluster tests should be a named task Former-commit-id: f715d0b860816165a2748ebf21876df87756a25a --- .github/workflows/ci.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 026d00592..478944397 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,6 +15,8 @@ jobs: run: | sudo apt-get -y install tcl8.5 ./runtest --clients 2 --verbose + - name: cluster-test + run: | ./runtest-cluster test-ubuntu-arm: @@ -29,6 +31,8 @@ jobs: run: | sudo apt-get -y install tcl8.5 ./runtest --clients 2 --verbose + - name: cluster-test + run: | ./runtest-cluster build-ubuntu-old: From 7ae66c08660216319a9819ba6db725a75c6dd224 Mon Sep 17 00:00:00 2001 From: Diab Neiroukh Date: Thu, 30 Apr 2020 18:43:23 +0100 Subject: [PATCH 13/24] Clang requires libatomic to be linked via LDFLAGS Clang requires the libatomic to be linked via LDFLAGS or else the linker will fail to recognise __atomic macros. An error as a result of not passing -latomic to the linker can be seen below: ld.lld: error: undefined symbol: __atomic_fetch_add_2 >>> referenced by sds.c:185 >>> lto.tmp:(sdsdupshared) clang-10: error: linker command failed with exit code 1 (use -v to see invocation) make[1]: *** [Makefile:276: keydb-server] Error 1 Former-commit-id: dcd7b684bbb7b4719a0e44d5764b01c48eb619dd --- src/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Makefile b/src/Makefile index bc60b4d74..4b4a996de 100644 --- a/src/Makefile +++ b/src/Makefile @@ -70,6 +70,7 @@ endif ifeq ($(COMPILER_NAME),clang) CXXFLAGS+= -stdlib=libc++ + LDFLAGS+= -latomic endif # To get ARM stack traces if Redis crashes we need a special C flag. From e28d3b33c7336f6dcc8c75b716ddc45766999d5d Mon Sep 17 00:00:00 2001 From: John Sully Date: Thu, 7 May 2020 01:49:04 -0400 Subject: [PATCH 14/24] Disable cluster tests They are unreliable on slow hardware. Former-commit-id: 86f9ed6248c6629af026e27d14d15a3eb50a2090 --- .github/workflows/ci.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 478944397..ef1333982 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,9 +31,6 @@ jobs: run: | sudo apt-get -y install tcl8.5 ./runtest --clients 2 --verbose - - name: cluster-test - run: | - ./runtest-cluster build-ubuntu-old: runs-on: ubuntu-16.04 From e9ecefdb7500087b61eee879bc40d4e9cdfabd95 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 10 May 2020 17:04:22 -0400 Subject: [PATCH 15/24] Implement keydb.hrename command Former-commit-id: 21d842b0b0d9a0da44e4618a2c1d4ac26553f17b --- src/server.cpp | 4 ++++ src/server.h | 1 + src/t_hash.cpp | 30 ++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/src/server.cpp b/src/server.cpp index c1966f968..10048519f 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -1042,6 +1042,10 @@ struct redisCommand redisCommandTable[] = { {"keydb.cron",cronCommand,-5, "write use-memory", 0,NULL,1,1,1,0,0,0}, + + {"keydb.hrename", hrenameCommand, 4, + "write fast @hash", + 0,NULL,0,0,0,0,0,0} }; /*============================ Utility functions ============================ */ diff --git a/src/server.h b/src/server.h index 9d13311e2..85da1fe9b 100644 --- a/src/server.h +++ b/src/server.h @@ -2966,6 +2966,7 @@ void xdelCommand(client *c); void xtrimCommand(client *c); void aclCommand(client *c); void replicaReplayCommand(client *c); +void hrenameCommand(client *c); int FBrokenLinkToMaster(); int FActiveMaster(client *c); diff --git a/src/t_hash.cpp b/src/t_hash.cpp index 25bb3b2e5..f1d893de6 100644 --- a/src/t_hash.cpp +++ b/src/t_hash.cpp @@ -832,3 +832,33 @@ void hscanCommand(client *c) { checkType(c,o,OBJ_HASH)) return; scanGenericCommand(c,o,cursor); } + +void hrenameCommand(client *c) { + robj *o = nullptr; + const unsigned char *vstr; + unsigned int vlen; + long long ll; + + if ((o = lookupKeyWriteOrReply(c,c->argv[1],shared.null[c->resp])) == nullptr || + checkType(c,o,OBJ_HASH)) return; + + if (hashTypeGetValue(o, szFromObj(c->argv[2]), &vstr, &vlen, &ll) != C_OK) + { + addReplyError(c, "hash key doesn't exist"); + return; + } + + sds sdsT = nullptr; + if (vstr != nullptr) + { + sdsT = sdsnewlen(vstr, vlen); + } + else + { + sdsT = sdsfromlonglong(ll); + } + + hashTypeDelete(o, szFromObj(c->argv[2])); + hashTypeSet(o, szFromObj(c->argv[3]), sdsT, HASH_SET_TAKE_VALUE); + addReplyLongLong(c, 1); +} \ No newline at end of file From ec82755227b12f4dd9337b23f15524c4ee5457a1 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 10 May 2020 17:14:44 -0400 Subject: [PATCH 16/24] hrename tests Former-commit-id: f77c227b2d34b7ec74c1fc993e03f063dcbfa090 --- tests/unit/type/hash.tcl | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/unit/type/hash.tcl b/tests/unit/type/hash.tcl index 9f8a21b1c..84f6aa646 100644 --- a/tests/unit/type/hash.tcl +++ b/tests/unit/type/hash.tcl @@ -445,6 +445,38 @@ start_server {tags {"hash"}} { } } + test {KEYDB.HRENAME basic} { + r flushall + r hset testkey foo bar + r keydb.hrename testkey foo foo1 + assert_equal [r hlen testkey] {1} + assert_equal [r hget testkey foo1] {bar} + } + + test {KEYDB.HRENAME same name} { + r flushall + r hset testkey foo bar + r keydb.hrename testkey foo foo + assert_equal [r hlen testkey] {1} + assert_equal [r hget testkey foo] {bar} + } + + test {KEYDB.HRENAME overwrite dest} { + r flushall + r hset testkey foo bar foo1 baz + r keydb.hrename testkey foo foo1 + assert_equal [r hlen testkey] {1} + assert_equal [r hget testkey foo1] {bar} + } + + test {KEYDB.HRENAME integer basic} { + r flushall + r hset testkey foo 1 + r keydb.hrename testkey foo bar + assert_equal [r hlen testkey] {1} + assert_equal [r hget testkey bar] {1} + } + test {Hash ziplist regression test for large keys} { r hset hash kkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk a r hset hash kkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk b From 318858eb4f48d5e3a27e2cc0c47518ec1d21b6f7 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 10 May 2020 17:41:01 -0400 Subject: [PATCH 17/24] add help for hrename Former-commit-id: 7e7f70ff3e238b0b70a40b3e0f0de4ba3c2720ea --- src/help.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/help.h b/src/help.h index d3870d490..000499505 100644 --- a/src/help.h +++ b/src/help.h @@ -1173,7 +1173,12 @@ struct commandHelp { "name [single/repeat] [optional: start] delay script numkeys [key N] [arg N]", "Run a specified script after start + delay, optionally repeating every delay interval. The job may be cancelled by deleting the key associated with the job (name parameter)", 10, - "6.5.2" + "6.5.2"}, + { "KEYDB.HRENAME", + "key [src hash key] [dst hash key]", + "Rename a hash key, copying the value.", + 4, + "6.5.3" } }; From 294f11a66d9cb15fb069722f36a2c387a5b959c7 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 10 May 2020 18:05:05 -0400 Subject: [PATCH 18/24] Run sentinel and module tests under CI Former-commit-id: 58752bb8feff60199a4351e5e659fd94ecfe3172 --- .github/workflows/ci.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ef1333982..b9bebf2e2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,13 @@ jobs: - name: cluster-test run: | ./runtest-cluster + - name: sentinel test + run: | + ./runtest-sentinel + - name: module tests + run: | + ./runtest-moduleapi + test-ubuntu-arm: runs-on: [self-hosted, linux, arm] @@ -31,6 +38,9 @@ jobs: run: | sudo apt-get -y install tcl8.5 ./runtest --clients 2 --verbose + - name: module tests + run: | + ./runtest-moduleapi build-ubuntu-old: runs-on: ubuntu-16.04 From 3c43d8109835e5036f1f2a75309d1e3cca3e9e59 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 10 May 2020 21:06:38 -0400 Subject: [PATCH 19/24] Fix crash in module tests Former-commit-id: 37423757b54b2052512dcfeaba72ccbd360d3c1e --- .gitmodules | 3 +++ deps/depot_tools | 1 + src/module.cpp | 4 ++-- 3 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 .gitmodules create mode 160000 deps/depot_tools diff --git a/.gitmodules b/.gitmodules new file mode 100644 index 000000000..5c2d13b36 --- /dev/null +++ b/.gitmodules @@ -0,0 +1,3 @@ +[submodule "deps/depot_tools"] + path = deps/depot_tools + url = https://chromium.googlesource.com/chromium/tools/depot_tools.git diff --git a/deps/depot_tools b/deps/depot_tools new file mode 160000 index 000000000..aaf566999 --- /dev/null +++ b/deps/depot_tools @@ -0,0 +1 @@ +Subproject commit aaf566999558aa8ead38811228cd539a6e6e2fda diff --git a/src/module.cpp b/src/module.cpp index 95116db77..c66701a9a 100644 --- a/src/module.cpp +++ b/src/module.cpp @@ -4701,13 +4701,14 @@ void moduleHandleBlockedClients(int iel) { if ((c != nullptr) && (iel != c->iel)) continue; + std::unique_lock ul; listDelNode(moduleUnblockedClients,ln); pthread_mutex_unlock(&moduleUnblockedClientsMutex); if (c) { AssertCorrectThread(c); - fastlock_lock(&c->lock); + ul = std::unique_lock(c->lock); } /* Release the lock during the loop, as long as we don't @@ -4773,7 +4774,6 @@ void moduleHandleBlockedClients(int iel) { /* Free 'bc' only after unblocking the client, since it is * referenced in the client blocking context, and must be valid * when calling unblockClient(). */ - fastlock_unlock(&c->lock); bc->module->blocked_clients--; zfree(bc); From 32801484a772eddde8ab228bcb840faa83880420 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 10 May 2020 21:57:16 -0400 Subject: [PATCH 20/24] Fix crash during hook module test Former-commit-id: 628d168049d00526169d13e31f540820aed1437c --- src/module.cpp | 10 +++++++--- src/redismodule.h | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/module.cpp b/src/module.cpp index c66701a9a..60420686e 100644 --- a/src/module.cpp +++ b/src/module.cpp @@ -1775,15 +1775,15 @@ int modulePopulateReplicationInfoStructure(void *ri, int structver) { memset(ri1,0,sizeof(*ri1)); ri1->version = structver; ri1->master = listLength(g_pserver->masters) == 0; - if (ri1->master) + if (!ri1->master) { - redisMaster *mi = (redisMaster*)listFirst(g_pserver->masters); + redisMaster *mi = (redisMaster*)listNodeValue(listFirst(g_pserver->masters)); ri1->masterhost = (char*)(mi->masterhost? mi->masterhost: ""); ri1->masterport = mi->masterport; } else { - ri1->masterhost = nullptr; + ri1->masterhost = ""; ri1->masterport = -1; } ri1->repl1_offset = g_pserver->master_repl_offset; @@ -7765,8 +7765,12 @@ int RM_GetLFU(RedisModuleKey *key, long long *lfu_freq) { *lfu_freq = -1; if (!key->value) return REDISMODULE_ERR; + serverLog(LL_WARNING, "MAXMEMORY_POLICY: %X", g_pserver->maxmemory_policy); if (g_pserver->maxmemory_policy & MAXMEMORY_FLAG_LFU) + { *lfu_freq = LFUDecrAndReturn(key->value); + serverLog(LL_WARNING, "lfu_freq: %lld", lfu_freq); + } return REDISMODULE_OK; } diff --git a/src/redismodule.h b/src/redismodule.h index fcd12e66a..3f08df509 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -324,7 +324,7 @@ typedef struct RedisModuleReplicationInfo { from the module to the core right now. Here for future compatibility. */ int master; /* true if master, false if replica */ - char *masterhost; /* master instance hostname for NOW_REPLICA */ + const char *masterhost; /* master instance hostname for NOW_REPLICA */ int masterport; /* master instance port for NOW_REPLICA */ char *replid1; /* Main replication ID */ char *replid2; /* Secondary replication ID */ From dc131afe1cadbd40442aaae29fdfea37236b7891 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 10 May 2020 22:08:50 -0400 Subject: [PATCH 21/24] Fix crash in client unblock command Former-commit-id: 7cd779d304d75833ca891b4fe3b7e1cfdabf6fa8 --- src/networking.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/networking.cpp b/src/networking.cpp index 1cb9eade6..6ff41d1ab 100644 --- a/src/networking.cpp +++ b/src/networking.cpp @@ -2779,6 +2779,7 @@ NULL != C_OK) return; struct client *target = lookupClientByID(id); if (target && target->flags & CLIENT_BLOCKED) { + std::unique_lock ul(target->lock); if (unblock_error) addReplyError(target, "-UNBLOCKED client unblocked via CLIENT UNBLOCK"); From f0ed02448d2e63fc9c3feed0c69602b8fbfa673f Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 10 May 2020 23:14:15 -0400 Subject: [PATCH 22/24] Fix race on clients_to_close Former-commit-id: 10fa06410941121c4c9fc3cc9c553a7afe347147 --- src/networking.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/networking.cpp b/src/networking.cpp index 6ff41d1ab..5ff886cf9 100644 --- a/src/networking.cpp +++ b/src/networking.cpp @@ -1590,6 +1590,8 @@ bool freeClient(client *c) { return true; } +fastlock lockasyncfree {"async free lock"}; + /* Schedule a client to free it at a safe time in the serverCron() function. * This function is useful when we need to terminate a client but we are in * a context where calling freeClient() is not possible, because the client @@ -1606,6 +1608,7 @@ void freeClientAsync(client *c) { lock.arm(c); if (c->flags & CLIENT_CLOSE_ASAP || c->flags & CLIENT_LUA) return; // race condition after we acquire the lock c->flags |= CLIENT_CLOSE_ASAP; + std::unique_lock ul(lockasyncfree); listAddNodeTail(g_pserver->clients_to_close,c); } @@ -1617,6 +1620,7 @@ void freeClientsInAsyncFreeQueue(int iel) { // Store the clients in a temp vector since freeClient will modify this list std::vector vecclientsFree; + std::unique_lock ul(lockasyncfree); while((ln = listNext(&li))) { client *c = (client*)listNodeValue(ln); @@ -1626,6 +1630,7 @@ void freeClientsInAsyncFreeQueue(int iel) { listDelNode(g_pserver->clients_to_close, ln); } } + ul.unlock(); for (client *c : vecclientsFree) { @@ -2393,11 +2398,13 @@ void readQueryFromClient(connection *conn) { return; } else { serverLog(LL_VERBOSE, "Reading from client: %s",connGetLastError(c->conn)); + aelock.arm(c); freeClientAsync(c); return; } } else if (nread == 0) { serverLog(LL_VERBOSE, "Client closed connection"); + aelock.arm(c); freeClientAsync(c); return; } else if (c->flags & CLIENT_MASTER) { From 012228bd51b237d45c4cbd8ef64225a5539b202a Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 10 May 2020 23:56:29 -0400 Subject: [PATCH 23/24] We should still send replies even if an async close is pending Former-commit-id: 572bcca9c2e7985782909cca22ef9e381bea55b4 --- src/networking.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.cpp b/src/networking.cpp index 5ff886cf9..dd2928879 100644 --- a/src/networking.cpp +++ b/src/networking.cpp @@ -1082,7 +1082,7 @@ void copyClientOutputBuffer(client *dst, client *src) { /* Return true if the specified client has pending reply buffers to write to * the socket. */ int clientHasPendingReplies(client *c) { - return (c->bufpos || listLength(c->reply)) && !(c->flags & CLIENT_CLOSE_ASAP); + return (c->bufpos || listLength(c->reply)); } static std::atomic rgacceptsInFlight[MAX_EVENT_LOOPS]; From 5bdcb8fefb6df8a07ac373b02ffa5c521b4a6206 Mon Sep 17 00:00:00 2001 From: John Sully Date: Mon, 11 May 2020 00:42:46 -0400 Subject: [PATCH 24/24] Update access LFU/LRU access times when overwriting key Former-commit-id: f11fdf671700fd5445599c473d69e015eb6618e8 --- src/db.cpp | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/db.cpp b/src/db.cpp index 01836d713..81e32f849 100644 --- a/src/db.cpp +++ b/src/db.cpp @@ -65,6 +65,22 @@ void updateExpire(redisDb *db, sds key, robj *valOld, robj *valNew) return; } +void updateDbValAccess(dictEntry *de, int flags) +{ + robj *val = (robj*)dictGetVal(de); + + /* Update the access time for the ageing algorithm. + * Don't do it if we have a saving child, as this will trigger + * a copy on write madness. */ + if (!hasActiveChildProcess() && !(flags & LOOKUP_NOTOUCH)){ + if (g_pserver->maxmemory_policy & MAXMEMORY_FLAG_LFU) { + updateLFU(val); + } else { + val->lru = LRU_CLOCK(); + } + } +} + /* Low level key lookup API, not actually called directly from commands * implementations that should instead rely on lookupKeyRead(), @@ -74,16 +90,7 @@ static robj *lookupKey(redisDb *db, robj *key, int flags) { if (de) { robj *val = (robj*)dictGetVal(de); - /* Update the access time for the ageing algorithm. - * Don't do it if we have a saving child, as this will trigger - * a copy on write madness. */ - if (!hasActiveChildProcess() && !(flags & LOOKUP_NOTOUCH)){ - if (g_pserver->maxmemory_policy & MAXMEMORY_FLAG_LFU) { - updateLFU(val); - } else { - val->lru = LRU_CLOCK(); - } - } + updateDbValAccess(de, flags); if (flags & LOOKUP_UPDATEMVCC) { val->mvcc_tstamp = getMvccTstamp(); @@ -312,6 +319,7 @@ void genericSetKey(redisDb *db, robj *key, robj *val, int keepttl) { if (de == NULL) { dbAdd(db,key,val); } else { + updateDbValAccess(de, LOOKUP_NONE); dbOverwriteCore(db,de,key,val,!!g_pserver->fActiveReplica,!keepttl); } incrRefCount(val);