From 67b70a18130bb22663494689b19db5b47cfed4fd Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 8 Jan 2016 15:05:14 +0100 Subject: [PATCH] Fix ae.c to avoid timers infinite loop. This fix was suggested by Anthony LaTorre, that provided also a good test case that was used to verify the fix. The problem with the old implementation is that, the time returned by a timer event (that is the time after it want to run again) is added to the event *start time*. So if the event takes, in order to run, more than the time it says it want to be scheduled again for running, an infinite loop is triggered. --- src/ae.c | 51 ++++++++++++++++++++--------------------------- src/ae.h | 3 +++ src/anet.h | 2 ++ src/redis-trib.rb | 2 +- 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/src/ae.c b/src/ae.c index 63a1ab4e..79fcde62 100644 --- a/src/ae.c +++ b/src/ae.c @@ -221,21 +221,12 @@ long long aeCreateTimeEvent(aeEventLoop *eventLoop, long long milliseconds, int aeDeleteTimeEvent(aeEventLoop *eventLoop, long long id) { - aeTimeEvent *te, *prev = NULL; - - te = eventLoop->timeEventHead; + aeTimeEvent *te = eventLoop->timeEventHead; while(te) { if (te->id == id) { - if (prev == NULL) - eventLoop->timeEventHead = te->next; - else - prev->next = te->next; - if (te->finalizerProc) - te->finalizerProc(eventLoop, te->clientData); - zfree(te); + te->id = AE_DELETED_EVENT_ID; return AE_OK; } - prev = te; te = te->next; } return AE_ERR; /* NO event with the specified ID found */ @@ -270,7 +261,7 @@ static aeTimeEvent *aeSearchNearestTimer(aeEventLoop *eventLoop) /* Process time events */ static int processTimeEvents(aeEventLoop *eventLoop) { int processed = 0; - aeTimeEvent *te; + aeTimeEvent *te, *prev; long long maxId; time_t now = time(NULL); @@ -291,12 +282,28 @@ static int processTimeEvents(aeEventLoop *eventLoop) { } eventLoop->lastTime = now; + prev = NULL; te = eventLoop->timeEventHead; maxId = eventLoop->timeEventNextId-1; while(te) { long now_sec, now_ms; long long id; + /* Remove events scheduled for deletion. */ + if (te->id == AE_DELETED_EVENT_ID) { + aeTimeEvent *next = te->next; + if (prev == NULL) + eventLoop->timeEventHead = te->next; + else + prev->next = te->next; + if (te->finalizerProc) + te->finalizerProc(eventLoop, te->clientData); + zfree(te); + te = next; + continue; + } + + /* Don't process time events created by time events in this iteration. */ if (te->id > maxId) { te = te->next; continue; @@ -310,28 +317,14 @@ static int processTimeEvents(aeEventLoop *eventLoop) { id = te->id; retval = te->timeProc(eventLoop, id, te->clientData); processed++; - /* After an event is processed our time event list may - * no longer be the same, so we restart from head. - * Still we make sure to don't process events registered - * by event handlers itself in order to don't loop forever. - * To do so we saved the max ID we want to handle. - * - * FUTURE OPTIMIZATIONS: - * Note that this is NOT great algorithmically. Redis uses - * a single time event so it's not a problem but the right - * way to do this is to add the new elements on head, and - * to flag deleted elements in a special way for later - * deletion (putting references to the nodes to delete into - * another linked list). */ if (retval != AE_NOMORE) { aeAddMillisecondsToNow(retval,&te->when_sec,&te->when_ms); } else { - aeDeleteTimeEvent(eventLoop, id); + te->id = AE_DELETED_EVENT_ID; } - te = eventLoop->timeEventHead; - } else { - te = te->next; } + prev = te; + te = te->next; } return processed; } diff --git a/src/ae.h b/src/ae.h index 15ca1b5e..827c4c9e 100644 --- a/src/ae.h +++ b/src/ae.h @@ -33,6 +33,8 @@ #ifndef __AE_H__ #define __AE_H__ +#include + #define AE_OK 0 #define AE_ERR -1 @@ -46,6 +48,7 @@ #define AE_DONT_WAIT 4 #define AE_NOMORE -1 +#define AE_DELETED_EVENT_ID -1 /* Macros */ #define AE_NOTUSED(V) ((void) V) diff --git a/src/anet.h b/src/anet.h index 8740a95d..7142f78d 100644 --- a/src/anet.h +++ b/src/anet.h @@ -31,6 +31,8 @@ #ifndef ANET_H #define ANET_H +#include + #define ANET_OK 0 #define ANET_ERR -1 #define ANET_ERR_LEN 256 diff --git a/src/redis-trib.rb b/src/redis-trib.rb index a38a01ea..8bb4abd5 100755 --- a/src/redis-trib.rb +++ b/src/redis-trib.rb @@ -941,7 +941,7 @@ class RedisTrib source.r.client.call(["migrate",target.info[:host],target.info[:port],"",0,@timeout,:replace,:keys,*keys]) else puts "" - xputs "[ERR] #{e}" + xputs "[ERR] Calling MIGRATE: #{e}" exit 1 end end