New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ALL simple issue fixes #1906
ALL simple issue fixes #1906
Conversation
Previously, if you did SELECT then AUTH, redis-cli would show your SELECT'd db even though it didn't happen. Note: running into this situation is a (hopefully) very limited used case of people using multiple DBs and AUTH all at the same time. Fixes antirez#1639
Previously redis-cli would happily show "-1" or "99999" as valid DB choices. Now, if the SELECT call returned an error, we don't update the DB number in the CLI. Inspired by @anupshendkar in redis#1313 Fixes redis#566, redis#1313
Some people need formatted output even when they have no interactive tty. Fixes redis#760
We only want to use the last STORE key, but we have to record we actually found a STORE key so we can increment the final return key count. Test added to prevent further regression. Closes redis#1883, redis#1645, redis#1647
Previously the end was casted to a smaller type which resulted in a wrong check and failed with values larger than handled by unsigned. Closes redis#1847, redis#1844
Negative key count causes segfault in Lua functions. Fixes redis#1842 Closes redis#1843
Cluster leaks memory while connecting due to missing freeaddrinfo() Closes redis#1801
(Cleaned up a little by @mattsta) Closes redis#1774
The classic (min+max)/2 is provably unsafe. Fixed as recommended in research: http://googleresearch.blogspot.com/2006/06/extra-extra-read-all-about-it-nearly.html Fix inspired by @wjin, but I used a different approach. (later, I found @kuebler fixed the same issue too). Fixes redis#1741, redis#1602
This was discovered by _bodya and reported in the IRC channel. Everything worked fine as these scripts are always executed as shell scripts. Closes redis#1728
dictAdd returns DICT_OK, not REDIS_OK. They both have the same underlying values, so it works even though the code is technically wrong. Fixes redis#1512
According to unix manuals, "Connecting to the socket object requires read/write permission." -- mode 755 is useless for anybody other than the owner. Fixes redis#1696
Use constants to avoid magic numbers in `types`, which is an array that stores the names of `REDIS` types. Closes redis#1681
No options are set for SIGTERM, so we can use the simpler signal handling interface and remove unnecessary structure initilization. sigemptyset() is moved outside of the ifdef to prevent any potential unused variable warning. Closes redis#1637
According to the C standard, it is desirable to give the type 'void' to functions have no argument. Closes redis#1631
Cleans up the flow of code to read much easier too. Closes redis#1615
Also adds test for numsub — due to tcl being tcl, it doesn't capture the "numberness" of the fix, but now we at least have one test case for numsub. Closes redis#1561
The replica instance will immediately sync with a master anyway, so we don't need to load anything from disk. If we add support for PSYNC after instance restart, we will need different logic here anyway. Closes redis#1543
[I had to split out the clang check due to clang *really* not liking the __GLIBC_PREREQ macro; -matt] Closes redis#1456
Previously, "MOVE key somestring" would move the key to DB 0 which is just unexpected and wrong. String as DB == error. Test added too. Fixes redis#1428
Improvements: - Return error if asking for bad section (INFO foo) - Fix potential memory leak (caused by sdsempty() then returned if >2 args) - Clean up argument parsing - Allow "all" as valid section (same as "default" or zero args currently) - Move strcasecmp to end of evaluation chain in conditionals Also, since we're C99, I moved some variable declarations to be closer to where they are actually used (saves us from needing to free an empty info if detect argument errors up front). Closes redis#1915
We don't want scripts doing CLIENT SETNAME or CLIENT KILL or CLIENT LIST or CLIENT PAUSE. Originally reported by Chris Wj then proper action inspired by Itamar Haber. Reference: https://groups.google.com/forum/#!topic/redis-db/09B2EYwyVgk
You're a crazy crazy man... ❤️ |
This is pretty impressive, thanks :-) I'll likely not be able to merge every single commit as I already spotted a few things I would do differently, so this is what I'll do: merge as much stuff as possible and rewrite all the obvious stuff in a branch called |
Sounds good to me. Looking forward to getting as much as possible merged. |
(Note: this comment will be updated incrementally)
Commits merged
Note no longer logging the commits merged since it's simpler to check the Commits that cannot be merged
Commits that need to be re-checked
|
Comments on non-merges
Good call. I didn't consider rewrite dropping those comments.
Update: You said the magic word "Solaris!" A quick search tells us: So, we need to keep the verbose "almost uselessly set all defaults" code in there. We probably want to shift
redis.c:1702: signal(SIGHUP, SIG_IGN);
redis.c:1703: signal(SIGPIPE, SIG_IGN);
Perhaps loading old data on startup should only happen if Or we could add better documentation about the bring-up state so people know: is old data fully loaded before new replication starts? If old data is being loaded when new replication starts, does it immediately abort loading old data?
Is it broken? We may need a better word than 'broken' for something that works but is just unwanted. All the tests pass for me :) If it is actually broken, we probably need better tests to detect the failures. :squirrel:
But, that 127.0.0.1:6379> mset a 1 b
(error) ERR wrong number of arguments for MSET
The
If we have useless checks in popular code, people will discover it and want to factor it out. :)
That's almost what I did the first time too. But then I tried to tried to make it match the original error message format where the command was upper case. Either way works. :)
Good catch! We didn't notice the default value always existed. Adding another config option seems a little redundant. We can fix the behavior of initializing Otherwise, if we add another config option, we'd have config options of A quick patch (combined with some changes from the existing 4a799b6) for the fix seems to be: diff --git a/src/redis.c b/src/redis.c
index 6400259..be7071c 100644
--- a/src/redis.c
+++ b/src/redis.c
@@ -1420,7 +1420,7 @@ void initServerConfig() {
server.aof_selected_db = -1; /* Make sure the first time will not match */
server.aof_flush_postponed_start = 0;
server.aof_rewrite_incremental_fsync = REDIS_DEFAULT_AOF_REWRITE_INCREMENTAL_FSYNC;
- server.pidfile = zstrdup(REDIS_DEFAULT_PID_FILE);
+ server.pidfile = NULL;
server.rdb_filename = zstrdup(REDIS_DEFAULT_RDB_FILENAME);
server.aof_filename = zstrdup(REDIS_DEFAULT_AOF_FILENAME);
server.requirepass = NULL;
@@ -3554,7 +3554,10 @@ int main(int argc, char **argv) {
}
if (server.daemonize) daemonize();
initServer();
- if (server.daemonize) createPidFile();
+ if (server.daemonize || server.pidfile) {
+ if (!server.pidfile) server.pidfile = zstrdup(REDIS_DEFAULT_PID_FILE);
+ createPidFile();
+ }
redisSetProcTitle(argv[0]);
redisAsciiArt(); We could even set the default [will be updated to defend positions as needed 🙀] |
@mattsta about:
What I mean is that it will perform invalid write into into not allocated memory. The MSET argument check passes since it's "at least N args", so when the command has to be dispatched by Redis Cluster, or when the command is processed by the COMMAND command, it writes into memory which is technically unallocated. The fact that it passes tests is that the allocator will never allocate 6 bytes, but 8, for alignment/padding concerns, but still the code is broken. Broken is not an offense, many of my code is / was broken, and many code submitted as pull requests may be broken. It is just an adjective to say a patch is not correct, and the contrary is "sane". These are pretty standard words in some coding circles, for example the Linux kernel mailing list. |
@mattsta about:
It is fair that the patch was submitted, but why we should give away a small bit of defensive programming because a PR was submitted? |
@mattsta about bb8ce05: You said:
As I pointed out in a detailed way in my original comment, the most critical aspect with this patch is that it makes failover ineffective in a crash-recovery scenario:
Being a replica of a master is a contract that should try to provide certain guarantees. Slaves can't just give away on the dataset because they are trying to connect again with a master. |
Btw replying to the technical issues above I believe I did not replied to your critique about my attitude, so it is better to explain a few things that are important IMHO for Redis / developers interaction.
|
Use unsigned length in sds header (commit 3f8f158) is interesting and deserves some discussion / care about a few things. It is also a great example of when my code is broken, since the signed integers were just accomplishing to waste one bit.
|
Should PR #1930 be added to this? |
@JamesS237 your mention of the issue is sufficient to add it to my processing list, thanks :-) |
The "command only checked for >= N args" is the part I was missing. I was (for some reason? thinking the check for Would it make sense to move length/step calculation up to command processing (where arity is checked) and outside of individual command functions?
With the combination of Redis being very popular and very open source, people will re-discover the same things that look like problems, even if they aren't. Kinda like how people keep submitting requests to add proper markdown to the README. I feel bad wasting multiple people's time re-submitting the same issue that's just unwanted.
Yup. No need to change system behavior here. It felt like a correctness optimization to me but I was wrong. :)
Yup. I was just temporarily blind to the case where it could happen, so I couldn't force it to do incorrect things.
We'll just have to agree to disagree there. Of course no test suite is absolutely complete, but comprehensive testing seems to be the best tool for outside contributors to verify changes to existing behaviors. If people can break the system, but have all tests pass, things get confusing. Though, since we know no testing is 100% perfect, that's why we do develop->test->review and not develop->release. Well, most of the time.
Yup, no problem reviewing. I'm just working from my own (sometimes broken) idea of how things work, so just need more education most times. Happy Tuesday! 💃 |
Some commands have non trivial logic and we never found some reason to change the current setup so far. Btw if we want to fix the problem of MSET allocating 2x the memory for key extraction, we can do so just by fixing the code in a way that makes sure we don't add edge cases. Probably it is enough to add (step-1) to argc before the division to never under-allocate.
Sometimes we may be able to fix this just adding a small comment.
I would love to reach a level of testing where if the test passes, it is very likely that the patch is sane, however even after spending a considerable amount of efforts in testing, we are not near this point at all, and I believe we'll never be with the current setup of two core developers. I strongly believe that when there are scarse development resources, the best idea is to try to write correct code beforehand and carefully review patches, instead of investing a huge amount of time writing more tests that will anyway not cover enough code and states to provide a good degree of safety. I think that there is currently no open source system software that can claim a degree of testing high enough that running the test is enough to merge a patch without manual inspection of the code. Probably the nearest system is Sqlite btw, so I'm curious about asking the core developers if they review or not the patches manually and how likely is a code breakage when merging something that passes all the tests but was not reviewed. We need to keep adding more and more tests, but it's just a safety net. |
Ok, everything I was able to merge is now merged, now merging all the stuff into |
Thanks! Every issue referenced here is now closed or has a note added about needing more work. We're down to under 200 pull requests now! :) |
Yey! Awesome! Thanks @mattsta for that hard work, thanks @antirez for merging so much stuff. (Btw., looking at really old PRs, there are definitely some that can be closed too) |
Thanks @mattsta, the backport process has finished, I merged everything into 3.0 and everything applicable into 2.8 (basically only the cluster stuff were not merged). I'll wait a few weeks for things to settle before doing a new 2.8 release. |
This is great! Maybe #1902 could be merged as well before the new release? |
<ChangeLog> --[ Redis 3.0.0 RC1 (version 2.9.101) ] Release date: 9 oct 2014 This is the first release candidate of Redis Cluster. >> General changes * [FIX] An very large number of small fixes, old and new, merged in the context of a the issue #1906. Please see the issue page here for exact credits: redis/redis#1906 of each commit. (Matt Stancliff and many others). * [FIX] SAVE is no longer propagated to AOF / slaves. * [FIX] GETRANGE test no longer fails for 32 bit builds (Matt Stancliff). * [FIX] Limit SCAN latency when the hash table is in an odd state (very few populted buckets because rehashing is in progress). (Xiaost and Salvatore Sanfilippo) * [NEW] Redis is now able to load truncated AOF files without requiring a redis-check-aof utility run. The default now is to load truncated (but apparently not corrupted) AOFs, you can change this in redis.conf. (Salvatore Sanfilippo). * [NEW] DEBUG POPULATE two args form implemented. It is now possible to call it with DEBUG POPULATE <count> <prefix>. Default prefix is "key:" as usually. * [NEW] INCR: Modify incremented object in-place when possible. This results in speed improvements + possibly better memory locality. >> Cluster changes * [FIX] Cluster: claim ping_sent time even if we can't connect. * [FIX] redis-trib should not abort easily on connection issues. * [FIX] Cluster test: less console-spammy resharding test. * [FIX] Fix logic to detect we are among a minority. * [FIX] Process gossip section only for known nodes. * [NEW] Redis Cluster is stable and tested enough, there is a clear MVP, so it was promoted from beta to stable. * [NEW] New unit 09, Pub/Sub across the cluster. * [NEW] New unit 08, update messages. * [NEW] New cluster option to work with partial slots coverage. * [NEW] More chatty cluster slaves when failover is stalled. They log reason with rate limiting, only when reason changes or a given time has elapsed. >> Sentinel changes * [FIX] Sentinel critical bug fixed: the absolute majority was computed in a wrong way because of a programming error. Now the implementation does what the specification says and the majority to authorize a failover (that should not be confused with the ODOWN quorum) is the majority of *all* the Sentinels ever seen for a given master, regardless of their current state. * [FIX] Resolved a memory leak in the hiredis library causing a memory leak in Redis Sentinel when a monitored instance or another Sentinel is unavailable. Every reconnection attempt will leak a small amount of memory, but in the long run the process can reach a considerable size. * [NEW] Sentinel: ability to announce itself with an arbitrary IP/port to work in the context of natted networks. However this is probably still not enough since there is no equivalent mechanism for slaves listed in the master INFO output. (Dara Kong and Salvatore Sanfilippo) </ChangeLog> git-svn-id: svn+ssh://svn.freebsd.org/ports/head@370602 35697150-7ecd-e111-bb59-0022644237b5
<ChangeLog> --[ Redis 3.0.0 RC1 (version 2.9.101) ] Release date: 9 oct 2014 This is the first release candidate of Redis Cluster. >> General changes * [FIX] An very large number of small fixes, old and new, merged in the context of a the issue #1906. Please see the issue page here for exact credits: redis/redis#1906 of each commit. (Matt Stancliff and many others). * [FIX] SAVE is no longer propagated to AOF / slaves. * [FIX] GETRANGE test no longer fails for 32 bit builds (Matt Stancliff). * [FIX] Limit SCAN latency when the hash table is in an odd state (very few populted buckets because rehashing is in progress). (Xiaost and Salvatore Sanfilippo) * [NEW] Redis is now able to load truncated AOF files without requiring a redis-check-aof utility run. The default now is to load truncated (but apparently not corrupted) AOFs, you can change this in redis.conf. (Salvatore Sanfilippo). * [NEW] DEBUG POPULATE two args form implemented. It is now possible to call it with DEBUG POPULATE <count> <prefix>. Default prefix is "key:" as usually. * [NEW] INCR: Modify incremented object in-place when possible. This results in speed improvements + possibly better memory locality. >> Cluster changes * [FIX] Cluster: claim ping_sent time even if we can't connect. * [FIX] redis-trib should not abort easily on connection issues. * [FIX] Cluster test: less console-spammy resharding test. * [FIX] Fix logic to detect we are among a minority. * [FIX] Process gossip section only for known nodes. * [NEW] Redis Cluster is stable and tested enough, there is a clear MVP, so it was promoted from beta to stable. * [NEW] New unit 09, Pub/Sub across the cluster. * [NEW] New unit 08, update messages. * [NEW] New cluster option to work with partial slots coverage. * [NEW] More chatty cluster slaves when failover is stalled. They log reason with rate limiting, only when reason changes or a given time has elapsed. >> Sentinel changes * [FIX] Sentinel critical bug fixed: the absolute majority was computed in a wrong way because of a programming error. Now the implementation does what the specification says and the majority to authorize a failover (that should not be confused with the ODOWN quorum) is the majority of *all* the Sentinels ever seen for a given master, regardless of their current state. * [FIX] Resolved a memory leak in the hiredis library causing a memory leak in Redis Sentinel when a monitored instance or another Sentinel is unavailable. Every reconnection attempt will leak a small amount of memory, but in the long run the process can reach a considerable size. * [NEW] Sentinel: ability to announce itself with an arbitrary IP/port to work in the context of natted networks. However this is probably still not enough since there is no equivalent mechanism for slaves listed in the master INFO output. (Dara Kong and Salvatore Sanfilippo) </ChangeLog>
<ChangeLog> --[ Redis 3.0.0 RC1 (version 2.9.101) ] Release date: 9 oct 2014 This is the first release candidate of Redis Cluster. >> General changes * [FIX] An very large number of small fixes, old and new, merged in the context of a the issue #1906. Please see the issue page here for exact credits: redis/redis#1906 of each commit. (Matt Stancliff and many others). * [FIX] SAVE is no longer propagated to AOF / slaves. * [FIX] GETRANGE test no longer fails for 32 bit builds (Matt Stancliff). * [FIX] Limit SCAN latency when the hash table is in an odd state (very few populted buckets because rehashing is in progress). (Xiaost and Salvatore Sanfilippo) * [NEW] Redis is now able to load truncated AOF files without requiring a redis-check-aof utility run. The default now is to load truncated (but apparently not corrupted) AOFs, you can change this in redis.conf. (Salvatore Sanfilippo). * [NEW] DEBUG POPULATE two args form implemented. It is now possible to call it with DEBUG POPULATE <count> <prefix>. Default prefix is "key:" as usually. * [NEW] INCR: Modify incremented object in-place when possible. This results in speed improvements + possibly better memory locality. >> Cluster changes * [FIX] Cluster: claim ping_sent time even if we can't connect. * [FIX] redis-trib should not abort easily on connection issues. * [FIX] Cluster test: less console-spammy resharding test. * [FIX] Fix logic to detect we are among a minority. * [FIX] Process gossip section only for known nodes. * [NEW] Redis Cluster is stable and tested enough, there is a clear MVP, so it was promoted from beta to stable. * [NEW] New unit 09, Pub/Sub across the cluster. * [NEW] New unit 08, update messages. * [NEW] New cluster option to work with partial slots coverage. * [NEW] More chatty cluster slaves when failover is stalled. They log reason with rate limiting, only when reason changes or a given time has elapsed. >> Sentinel changes * [FIX] Sentinel critical bug fixed: the absolute majority was computed in a wrong way because of a programming error. Now the implementation does what the specification says and the majority to authorize a failover (that should not be confused with the ODOWN quorum) is the majority of *all* the Sentinels ever seen for a given master, regardless of their current state. * [FIX] Resolved a memory leak in the hiredis library causing a memory leak in Redis Sentinel when a monitored instance or another Sentinel is unavailable. Every reconnection attempt will leak a small amount of memory, but in the long run the process can reach a considerable size. * [NEW] Sentinel: ability to announce itself with an arbitrary IP/port to work in the context of natted networks. However this is probably still not enough since there is no equivalent mechanism for slaves listed in the master INFO output. (Dara Kong and Salvatore Sanfilippo) </ChangeLog>
👽 DON'T PANIC 👽
This is every simple, non-controversial, single-issue fix I found in the issue/PR backlog.
Yes, this branch has
545763 commits that will auto-close over 50 issues when merged (#463, #513, #537, #811, #857, #858, #878, #952, #1188, #1187, #997, #1092, #1105, #1129, #1161, #1186, #1191, #1300, #1327, #1376, #1428, #1450, #1456, #1543, #1519, #1561, #1597, #1610, #1614, #1615, #1631, #1637, #1660, #1681, #1696, #1512, #1728, #1741, #1602, #1741, #1774, #1801, #1842, #1843, #1847, #1844, #1883, #1645, #1647, #1900, #760, #566, #1313, #521, #1649, #1912, #1909, #1914, #1097, #1915)Yes, this branch has
313234 authors (HI COMMUNITY! We haven't forgotten you!).Yes, I manually reviewed, edited, and/or fixed every commit here.
For quick browsing of all the changes, use this diff view: https://github.com/antirez/redis/pull/1906/files
Everything works, all tests pass (https://travis-ci.org/mattsta/redis/builds/31819638), and this Branch of Awesome Correctness is ready for merging into Actual Redis.
Notable changes include:
int
tounsigned int
replacement; everything still works)SHUTDOWN
cleanly instead of just killing the server.-a
option toredis-benchmark
to allow auth when testing