[10924][patch] Timer system improved

[Ambal]

Hi, everyone.

I want to try and fix our timer system so we can keep track time intervals between Update() calls for specific object and update spells/corpse/regen and many other timers accordingly. This should resolve issues we have with mob respawn on inactive grids, some spells like F@r S1ght etc.

Vladimir and ciphercom made several patches, the last commit was [10688], but unfortunately all of them have failed for several reasons:

1) sometimes, especially in earliest versions of patches, you can get HUGE diff time by unknown reasons... Well, the real problem was connected to the way we obtain timestamps to initialize time/timers. Here is an explanation why even [10688] has failed:

...

World::Update()

{

world_time = A; <= you start your world update at time A

}

...

World::UpdateSessions()

{

Map::create_time = B; <= during packet processing you create a player and add him to the map. Map decides that it was created at time B, where in 99% of cases B > A

}

...

Map::Update()

{

real_diff = getMSTimeDiff(create_time, world_time); where create_time = B, world_time = A and B > A <- this is where you get HUGE update

}

To resolve this issue we should save and use time when current WORLD update tick started to execute. This way you will NEVER get any issues with time tracking since your time interval is fixed now until next World::Update

2) there was a problem with diffs handled by grids. For example, you add an object to inactive grid, then wait for 7 mins and step into that grid making it active -> BANG, you get 7 mins DIFF time for your object! We will use a separate timer for each WorldObject to measure intervals between actual updates. With such approach situation described above is not possible at all.

I've used some parts of Vladimir and ciphercom's code to make a patch since code inside Unit/Player/Creature/***/::Update() functions is correct. Please, test, post comments and report any issues you find. Cheers

P.S. Give this patch a try - if it has any of previous issues, lets forget about real diffs in mangos

My repo is: https://github.com/Ambal/mangos

[Ambal]

As I see, everyone are so scared after commits [10688] and [10677] so noone is willing to test similar patches anymore Ok, let's go another way: if you want more patches from me, test this one

[kbz]

i got a question bout this: how are we supposed to test? what is the expected result before->after?

[Diablox]

i got a question bout this: how are we supposed to test? what is the expected result before->after?

Mobs should respawn in inactive grids.

[Ambal]

i got a question bout this: how are we supposed to test? what is the expected result before->after?

Killed unlooted mobs should disappear in inactive grids.

[maly32167]

what if someone havent grid unloading turned on ? does it make any difference ?

[Ambal]

what if someone havent grid unloading turned on ? does it make any difference ?

no, it should not make any difference, maly32167.

[kero99]

As I see, everyone are so scared after commits [10688] and [10677] so noone is willing to test similar patches anymore Ok, let's go another way: if you want more patches from me, test this one

I can test patch but need first some days to finish with Reloc branch1 test ... Soo much amazing patches to test in few days!!!! hehehe

[Ambal]

I can test patch but need first some days to finish with Reloc branch1 test ...

No hurry, I'm just checking that patches are not lost from community view We appreciate your effort in testing patches

[Ambal]

Patch updated: signature of Update(uint32 time_diff) functions has been changed to Update(uint32 update_diff, uint32 time_diff) to get rid of auxilary function WorldObject::updateDiff(). Patch supports latest revision. Have fun

[Undergarun]

Testing right now...

[Ambal]

Any test results for this patch? :rolleyes:

[VladimirMangos]

if (oldMSTime > newMSTime)
+        {
+            //don't try to push stupid code in mangos! This can't happen!!!
+            MANGOS_ASSERT(false);

This is just wrong as i think. mstime overflow is _correct_ thinks that can be happens after not so long uptime.

All code base at expect impossibility new time has been in raw values < old time is wrong.

[Ambal]

This is just wrong as i think. mstime overflow is _correct_ thinks that can be happens after not so long uptime.

Overflow is not a case for mangos since we changed the way how we obtain and calculate values in getMSTime() function. We use time diff to calculate uptime in miliseconds, you can check rev [10694]. If you'll find a place in code where suggested code change might cause problems, please, report and we'll think about a fix.

Also, please note that even old mangos did not had any protection against system timer changes on runtime, e.g. moving system clock forward/backward on server machine.

P.S. Overflow for getMSTime() will happen if your server uptime will reach 49+ days.

[VladimirMangos]

P.S. Overflow for getMSTime() will happen if your server uptime will reach 49+ days.

I refer to this. Not good add limitation and expections just because "512Kb memory will only need for any program run"

[Undergarun]

Sorry ambal but i have to revert this and other custom patches because of ACE crashes (Not related to this patch but i have to solve it).

Any help will be appreciated!

[Ambal]

I refer to this.

In this case we should consider using uint64 data type to store time diffs instead of uint32. Current approach is a bad design, IMO, and we can't leave it as is if we want to deal with overflows with high uptime of 49 days (which is HUGE value, almost a 1.5 month running in 24x7 mode ).

P.S. Lets be realistic: we should not try to design systems which are capable of doing everything and handle all possible and impossible situations. 49 days of uptime is unrealistic even for Blizz just because they do server maintenance every week e.g. 7(!) days disregarding of the fact how long their servers are capable to run. Regular uptime of mangos servers is even lower... Don't see a reason to bother. What we should really deal with - is moving system clock forward/backward - current core does not handle this situation in a correct way.

[Unkle Nuke]

Assuming there would be some crazy guy out there, who has a costly server rack, and has his MaNGOS running for 49+ days...

Could you not error trap an overflow by resetting the mstime when a certain value is reached? You could use the uptime value in the realmd database as a string when showing total running time. This would avoid having to convert to uint64.

[Ambal]

Assuming there would be some crazy guy out there, who has a costly server rack, and has his MaNGOS running for 49+ days....

Well, lets first meet such guy who will have a server running for 49 days with online > 0 players

Could you not error trap an overflow by resetting the mstime when a certain value is reached? This would avoid having to convert to uint64.

I use error trapping to test patch and see if we do not have situations where old_time > new_time (will happen ONLY if someone will move system clock backwards which I doubt dude in mind will do). Also, w/o using uint64 data type it is impossible to resolve overflow issue with high uptime in a correct way.

Current way of dealing with situations "old_time > new_time" is far from acceptable so we should consider a fix for it and not leave hacks around the code. Two patches with the same issue is the most obvious proof

P.S. Windoz has a well known function "DWORD GetTickCount()" which returns time in msecs from OS start time which is limited to the same 49 days as our current getMSTime() function. And I don't see complains about it, really

From MSDN: "The elapsed time is stored as a DWORD value. Therefore, the time will wrap around to zero if the system is run continuously for 49.7 days. To avoid this problem, use the GetTickCount64 function. Otherwise, check for an overflow condition when comparing times."

[VladimirMangos]

Old code in part that work with ticks have way avoid problem in overflow case. For this not ned use uint64, only need expect one limitations: compared values near each other, So let propertly proccess in bear overflow.

And please not use as point :2 reverted patches". Second has been reveted by flow in design not related to mstime calculation ways. In fact this part work in it after return to use another counters.

[Unkle Nuke]

Ok, but using Windows as an example is one method for uptime, but what about Linux or Unix? Those systems commonly have uptime counted in months.

How do they handle it?

[Ambal]

So let propertly proccess in bear overflow.

uint64 is the most reliable solution, IMO, which is not that hard to implement w/o modifying code alot. But we can improve our overflow check too. Anyway, things should not be bound to 49.7 days of uptime, should they? :lol:

[lp-vamp]

every variable is limited in size. The question is quite right. How much time diff would we like to count?

The 49 Days is quite huge but i can remember i've my server projekt online for one month with ~10ppl.

In my oppinion i would spend a larger range of time diff, larger than 49 days. In my programming skills i would try to catch the overflow and count tha to another variable. So at the end it would end in two uint32 = one uint64 varible.

uint64 sounds good to me

cheers

[DasBlub]

I use error trapping to test patch and see if we do not have situations where old_time > new_time (will happen ONLY if someone will move system clock backwards which I doubt dude in mind will do).

what about summer/winter time change?

[VladimirMangos]

Ok, maybe compromise sollution: use native int time. So at x86 platform will be 49 days limit and at x64 platform will lot lagger values. Anyway with time x64 used more wide.