Skip to content
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

BackgroundWorkers should be restartable to permit changes in Timer interval #19986

Merged
merged 3 commits into from
Jun 12, 2024
Merged

BackgroundWorkers should be restartable to permit changes in Timer interval #19986

merged 3 commits into from
Jun 12, 2024

Conversation

machonky
Copy link
Contributor

@machonky machonky commented Jun 6, 2024

As BackgroundWorkers are a Singleton, they should be restartable to permit changes in Timer interval without an application restart.

Description

When StopAsync is invoked followed by a StartAsync on a BackgroundWorker the StoppingTokenSource is not renewed and will throw if the BackgroundWorker is stopped a second time.

The change assigns a new CancellationTokenSource to the StoppingTokenSource property and assigns a new token from the new source.

Checklist

  • I fully tested it as developer / designer and created unit / integration tests
  • I documented it (or no need to document or I will create a separate documentation issue)

…terval

As BackgroundWorkers are a Singleton, they should be restartable to permit changes in Timer interval.
@CLAassistant
Copy link

CLAassistant commented Jun 6, 2024

CLA assistant check
All committers have signed the CLA.

@maliming maliming self-requested a review June 10, 2024 08:58
@maliming
Copy link
Member

hi

As BackgroundWorkers are a Singleton, they should be restartable to permit changes in Timer interval without an application restart.

Can you share some code to show this case?

The StopAsync is called on ApplicationShutdown. so it won't restart.

@machonky
Copy link
Contributor Author

machonky commented Jun 11, 2024

When the Host changes a system-wide polling interval setting a local message instructs the PerTenantBackgroundWorker to change as shown - Example code:

public class PerTenantBackgroundWorker : AsyncPeriodicBackgroundWorkerBase
{
...
    public async Task ChangePollingInterval(int newPollingInterval)
    {
        var oldValue = Timer.Period / Minutes;
        logger.LogInformation($"{ToString()} changing polling interval from {oldValue} to {newPollingInterval} (mins)");
        
        await StopAsync(StoppingToken);
        Timer.Period = newPollingInterval * Minutes;
        await StartAsync();

        logger.LogInformation($"{ToString()} Changed polling interval to: {newPollingInterval}");
    }
...
}

If the StopAsync is invoked after this without renewing the StoppingToken then an exception is thrown from the token source, as it has been used up already so it must be "renewed" after every stop call.

The change makes it routine to change the polling interval without needing an application restart,

@maliming
Copy link
Member

ok, We can make these properties to protected, then you can change them in the sub-class.

protected CancellationTokenSource StoppingTokenSource { get; set; }

protected CancellationToken StoppingToken { get; set; }

We can drop the ResetStoppingCancellationTokenSource method.

What do you think?

@machonky
Copy link
Contributor Author

Yes. Since StopAsync is overridable, this will work too.

@maliming
Copy link
Member

Can you update your PR's code? Thanks

@machonky
Copy link
Contributor Author

Updated: 9e051f6

@maliming
Copy link
Member

Thanks.

Can you remove these spaces?

image

@machonky
Copy link
Contributor Author

@maliming
Copy link
Member

Thanks!

@maliming maliming merged commit 556c0ad into abpframework:dev Jun 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants