Make the timer event list in sorted order && Fix for issue #633 #654

Open
wants to merge 2 commits into
from

Projects

None yet

3 participants

@jokea
jokea commented Aug 30, 2012

I can't split the commits into two pull requests, so put them in a single one.

What the first commit does:

Insert the timer event in sorted order, so it's O(1) to get
the nearest timer. Insert and delete a timer is O(N).

And the second:

When system time changes back, the timer will not worker properly
hence some core functionality of redis will stop working(e.g. replication,
bgsave, etc). 

The patch saves the previous time and when a system clock skew is detected,
it will force expire all timers.

See issue #633 for details of the second commit.

jokea added some commits Aug 30, 2012
@jokea jokea Insert the timer event in sorted order, so it's O(1) to get
the nearest timer. Insert and delete a timer is O(N).
941825c
@jokea jokea Force expire all timer events when system clock skew is detected.
When system time changes back, the timer will not worker properly
hence some core functionality of redis will stop working(e.g. replication,
bgsave, etc). See issue #633 for details.

The patch saves the previous time and when a system clock skew is detected,
it will force expire all timers.
4274554
@AnuragBerdia

Hi Jokea,
This may sound stupid, I have no idea how this(pull request) works in redis & github.
Any ideas, when will this be merged in redis main branch and released?

Thanks.

@antirez
Owner
antirez commented Sep 12, 2012

Hi, I put this issue in 2.6 milestone for review in the next days. Thanks.

@antirez
Owner
antirez commented Oct 4, 2012

Review started, I hope to merge today.

@antirez
Owner

The above 5 lines of code are probably not useful, because if eventLoop->timeEventHead is NULL, what happens in the next lines is:

  • prev = entry (so prev = entry = NULL)
  • while (entry ...) is not entered at all as entry is NULL
  • if (prev == entry) is entered as they are both NULL, and what happens is: te->text = entry; eventLoo->timeEventHead = te that is what the above 5 lines do.

So I guess we can remove this code. Makes sense?

@antirez
Owner
antirez commented Oct 4, 2012

Hi folks,

I think commit 4274554 is fine and fixes an actual issue so I want to merge it into 2.6 and unstable.

For commit 941825c things are a bit different, basically from the point of view of Redis that has just one timer it does not offer a real advantage, but my add bugs, I found one for instance proof reading the code: the processTimeEvents() function no longer checks for maxid to process (plus restarting from head again if an event is processed), so if an event fires a callback that alters the list (for instance deleting its own time event, the one that fired the procedure) bad things may happen.

At this point I'm cherry-picking commit 4274554 and moving the milestone of this issue to 2.8 so that we can have time to reimplement the O(1) thing with a skiplist and with more checks to make sure everything will be safe.

Thanks,
Salvatore

@antirez
Owner
antirez commented Oct 4, 2012

Another note, this time about commit 4274554, currently the code is thread safe, so I would like to retain this capability and add a last_sec field in the eventloop structure itself so that every event loop has its own private field. In this way it is possible to use the library with multiple event loops from multiple threads.

@antirez
Owner
antirez commented Oct 4, 2012

Moved to 2.8

@jokea
jokea commented Oct 9, 2012

I agree with you that line 173-177 can be safely removed.
About the O(1) commit, since redis has only one timer, so there's not any overhead without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment