-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix System.Timers.Timer Stop() not working after setting AutoReset or Interval on disabled one-shot timer #122843
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
base: main
Are you sure you want to change the base?
Conversation
…d one-shot timer Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
src/libraries/System.ComponentModel.TypeConverter/tests/TimerTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in System.Timers.Timer where Stop() would fail to stop the timer after setting AutoReset or Interval on a disabled one-shot timer. When a one-shot timer fires, it sets _enabled = false directly without nulling _timer, allowing subsequent property changes to inadvertently restart the timer while leaving _enabled false, causing Stop() to become ineffective.
Key Changes:
- Added
&& _enabledcheck in bothAutoResetandIntervalsetters before callingUpdateTimer() - Updated existing test to explicitly call
Start()after changingAutoReset - Added two new tests to verify the fix and ensure
Stop()works correctly
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.ComponentModel.TypeConverter/src/System/Timers/Timer.cs | Added && _enabled condition to AutoReset and Interval setters to prevent UpdateTimer() from being called when the timer is disabled |
| src/libraries/System.ComponentModel.TypeConverter/tests/TimerTests.cs | Updated TestTimerStartAutoReset to explicitly call Start(); added SettingAutoResetOrIntervalOnDisabledOneShotTimerDoesNotRestartIt test to verify disabled timers don't restart; added StopWorksAfterSettingAutoResetOrIntervalOnRunningTimer test to ensure Stop() works after property changes |
Comments suppressed due to low confidence (2)
src/libraries/System.ComponentModel.TypeConverter/tests/TimerTests.cs:146
- The sleep duration of 100ms may be insufficient to reliably verify that the timer did not restart, especially on slower or heavily loaded systems. Consider increasing the sleep duration to at least 200-500ms or using a more deterministic approach with a longer wait combined with assertion on the count remaining zero.
Thread.Sleep(100);
src/libraries/System.ComponentModel.TypeConverter/tests/TimerTests.cs:178
- The sleep duration of 100ms may be insufficient to reliably verify that the timer remained stopped, especially on slower or heavily loaded systems. Consider increasing the sleep duration to at least 200-500ms to reduce the likelihood of test flakiness.
Thread.Sleep(100);
src/libraries/System.ComponentModel.TypeConverter/tests/TimerTests.cs
Outdated
Show resolved
Hide resolved
…valOnRunningTimer test Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…ditions Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
| { | ||
| _autoReset = value; | ||
| if (_timer != null) | ||
| if (_timer != null && _enabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, I think it'd be better to move these checks into UpdateTimer itself; that way all callers of UpdateTimer get the right behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the _enabled check into UpdateTimer() itself. The method now returns early if _timer is null || !_enabled, so all callers get the right behavior. Fixed in 419c4d9.
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
main PR
Description
When a one-shot timer (
AutoReset=false) fires, it sets_enabled = falsedirectly without nulling_timer. Subsequently settingIntervalorAutoResetcallsUpdateTimer()because those setters only checkif (_timer != null). This reactivates the timer while_enabledremainsfalse, causingStop()to no-op.Fix: Modified
UpdateTimer()to check both_timerand_enabledbefore updating the timer. This centralizes the check so all callers get the right behavior automatically.Customer Impact
Timer cannot be stopped after changing
AutoResetorIntervalon a disabled one-shot timer, leading to runaway timers.Regression
No. This behavior has existed since .NET Framework.
Testing
System.ComponentModel.TypeConvertertests passTestTimerStartAutoResetto explicitly callStart()after changingAutoResetSettingAutoResetOrIntervalOnDisabledOneShotTimerDoesNotRestartIttest that verifiesEnabledstaysfalseafter setting properties on a disabled timer (deterministic, no timing-dependent assertions)Risk
Low. The fix is minimal and adds a defensive check in
UpdateTimer()that prevents unintended timer reactivation. Breaking change for code relying on the buggy implicit restart behavior—such code must now explicitly callStart().Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.