Skip to content

Commit

Permalink
Merge branch 'master' into alpha
Browse files Browse the repository at this point in the history
Fixes #264
  • Loading branch information
Lexikos committed Apr 10, 2022
2 parents 323b301 + 3be2c7a commit cf94eed
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 9 deletions.
27 changes: 19 additions & 8 deletions source/application.cpp
Expand Up @@ -1623,14 +1623,25 @@ bool CheckScriptTimers()
timer.mCallback->ExecuteInNewThread(_T("Timer"));
--timer.mExistingThreads;

// Resolve the next timer only now, in case other timers were created or deleted while
// this timer was executing. Must be done before the timer is potentially deleted below.
next_timer = timer.mNextTimer;
// Currently timers are disabled only when they can't be deleted (because they're
// running). So now that this one has finished, check if it needs to be deleted.
// mExistingThreads==0 is implied at this point since timers are only allowed one thread.
if (!timer.mEnabled)
g_script.DeleteTimer(timer.mCallback->ToObject());
for (auto *this_timer = &timer; this_timer; this_timer = next_timer)
{
// Resolve the next timer only now, in case other timers were created or deleted while
// this timer was executing. Must be done before the timer is potentially deleted below.
next_timer = this_timer->mNextTimer;
// Currently timers are disabled only when they can't be deleted (because they're
// running). So now that this one has finished, check if it needs to be deleted.
if (this_timer->mEnabled || this_timer->mExistingThreads || this_timer->mDeleteLocked)
break;
if (next_timer)
next_timer->mDeleteLocked++; // Prevent next_timer from being deleted.
// The following call can trigger __delete, which can cause further changes to timers,
// either directly via SetTimer or via thread interruption.
g_script.DeleteTimer(this_timer->mCallback->ToObject());
if (next_timer)
next_timer->mDeleteLocked--; // Might still be non-zero due to thread interruption.
// Now also check next_timer, in case it was disabled while __delete was executing.
} // for() series of timers being deleted.

} // for() each timer.

if (at_least_one_timer_launched) // Since at least one subroutine was run above, restore various values for our caller.
Expand Down
3 changes: 2 additions & 1 deletion source/script.cpp
Expand Up @@ -4133,7 +4133,8 @@ void Script::DeleteTimer(IObject *aLabel)
// Disable it, even if it's not technically being deleted yet.
if (timer->mEnabled)
timer->Disable(); // Keeps track of mTimerEnabledCount and whether the main timer is needed.
if (timer->mExistingThreads) // This condition differs from g->CurrentTimer == timer, which only detects the "top-most" timer.
if (timer->mExistingThreads // This condition differs from g->CurrentTimer == timer, which only detects the "top-most" timer.
|| timer->mDeleteLocked)
{
// In this case we can't delete the timer yet, but CheckScriptTimers()
// will check mEnabled after the callback returns and will delete it then.
Expand Down
2 changes: 2 additions & 0 deletions source/script.h
Expand Up @@ -1999,6 +1999,7 @@ class ScriptTimer
DWORD mTimeLastRun; // TickCount
int mPriority; // Thread priority relative to other threads, default 0.
UCHAR mExistingThreads; // Whether this timer is already running its subroutine.
UCHAR mDeleteLocked; // Lock count to prevent full deletion. Separate to mExistingThreads so it doesn't prevent timer execution.
bool mEnabled;
bool mRunOnlyOnce;
ScriptTimer *mNextTimer; // Next items in linked list
Expand All @@ -2008,6 +2009,7 @@ class ScriptTimer
: mCallback(aLabel), mPeriod(DEFAULT_TIMER_PERIOD), mPriority(0) // Default is always 0.
, mExistingThreads(0), mTimeLastRun(0)
, mEnabled(false), mRunOnlyOnce(false), mNextTimer(NULL) // Note that mEnabled must default to false for the counts to be right.
, mDeleteLocked(0)
{}
};

Expand Down

2 comments on commit cf94eed

@HelgeffegleH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lexikos
This can potentially skip every other timer with the following consequences,

  • The order of timer execution is changed
  • Timer precision is degraded (since only (about) half of the timers are consider per call to CheckScriptTimers())

While neither order nor precision is guaranteed, I guess we can probably avoid changes in behaviour with something like,

// Code not tested
next_timer = nullptr;
for (auto *this_timer = &timer ; this_timer; this_timer = next_timer)
{
	if (this_timer->mEnabled || this_timer->mExistingThreads || this_timer->mDeleteLocked)
	{
		if (!next_timer)
			next_timer = this_timer->mNextTimer;
		break;
	}
	next_timer = this_timer->mNextTimer;

The logic for avoiding ub seems solid 👍

@Lexikos
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without comments or any indication of how the timers are skipped, I found it difficult to see the logic of your code at first.

Commit 66b7f07

Please sign in to comment.