Investigation hanging audit instance #4758
Conversation
Co-authored-by: Ramon Smits <ramon.smits@gmail.com>
ramonsmits
force-pushed
the
fix-hanging-ingestion
branch
from
eb85f65 to
3f15bee
Compare
ramonsmits
force-pushed
the
fix-hanging-ingestion
branch
from
3f15bee to
fd0df00
Compare
ramonsmits
force-pushed
the
fix-hanging-ingestion
branch
from
fd0df00 to
6a26fdc
Compare
| } | ||
|
|
||
| public Task StartAsync(CancellationToken _) => watchdog.Start(() => applicationLifetime.StopApplication()); | ||
| public async Task StartAsync(CancellationToken cancellationToken) |
There was a problem hiding this comment.
@andreasohlund @danielmarbach Alternative is to use BackgroundService.ExecuteAsync, assuming we can safely terminate ASAP:
protected override async Task ExecuteAsync(CancellationToken stoppingToken)
{
await watchdog.Start(() => applicationLifetime.StopApplication());
await Loop(stoppingToken);
// Intentionally not invoking the following to shut down ASAP
// watchdog.Stop();
// channel.Writer.Complete();
// if (transportInfrastructure != null)
// {
// await transportInfrastructure.Shutdown(stoppingToken);
// }
}As everything is at-least-once processing and idempotent this would be the fastest method to shutdow.
There was a problem hiding this comment.
I think it makes sense to switch to a BackgroundService here and inline the loop into the execute method
Be aware though of dotnet/runtime#36063 and https://blog.stephencleary.com/2020/05/backgroundservice-gotcha-startup.html for more context
There was a problem hiding this comment.
Yes, I already tested that and it works. As the loop pretty much immediately does IO its not affected which is why I removed the Task.Run. I can restore it for safety or add a code comment. Any preference @danielmarbach ?
| catch (Exception e) // show must go on | ||
| { | ||
| if (logger.IsInfoEnabled) | ||
| catch (OperationCanceledException e) when (e.CancellationToken == cancellationToken) |
There was a problem hiding this comment.
@andreasohlund please review todays commits seperately and then combined.
Fixed by properly handling Stop/Start CancellationTokens and validating thrown OCE against host token and added fatal exception handling.
…duration to close within 30 seconds
ramonsmits
force-pushed
the
fix-hanging-ingestion
branch
from
21ffe9c to
d8d5730
Compare
| public async ValueTask<IAuditIngestionUnitOfWork> StartNew(int batchSize, CancellationToken cancellationToken) | ||
| { | ||
| var timedCancellationSource = new CancellationTokenSource(databaseConfiguration.BulkInsertCommitTimeout); | ||
| var timedCancellationSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); |
There was a problem hiding this comment.
It would be good to have a comment here indicating the disposal happens as part of the unit of work
| } | ||
|
|
||
| auditBatchSize.Record(contexts.Count); | ||
| var sw = Stopwatch.StartNew(); |
There was a problem hiding this comment.
Not relevant for this PR but we should be using a ValueStopWatch here
There was a problem hiding this comment.
Is that included somewhere or do we copy https://github.com/dotnet/aspnetcore/blob/main/src/Shared/ValueStopwatch/ValueStopwatch.cs ?
| } | ||
|
|
||
| public Task StartAsync(CancellationToken _) => watchdog.Start(() => applicationLifetime.StopApplication()); | ||
| public async Task StartAsync(CancellationToken cancellationToken) |
There was a problem hiding this comment.
I think it makes sense to switch to a BackgroundService here and inline the loop into the execute method
Be aware though of dotnet/runtime#36063 and https://blog.stephencleary.com/2020/05/backgroundservice-gotcha-startup.html for more context
…ion are managed in the same code
…tException are managed in the same code
ramonsmits
force-pushed
the
fix-hanging-ingestion
branch
from
e87f040 to
a2f471e
Compare
|
Closing this analysis PR which resulted in: Cancellation changes are part of: |
No description provided.