Hallo zusammen,
ich habe kürzlich hier meinen Audio-Tagger tone
(https://github.com/sandreas/tone) vorgestellt und nun ein (für mich) interessantes Problem. Und zwar hat mir jemand gemeldet, dass das Drücken von STRG+C inmitten der Veränderung der Metadaten dazu führt, dass Dateien "kaputtgeschrieben" werden, weil die Anwendung ohne Vorwarnung terminiert.
Nun habe ich mich etwas eingelesen und einen Vorschlag eines andren Entwicklers für die Kommandozeilen-Bibliothek spectre.console
dazu mehr oder minder unverändert übernommen (siehe hier https://github.com/spectreconsole/spectre.console/issues/701#issuecomment-1081834778):
internal class ConsoleAppCancellationTokenSource
{
private readonly CancellationTokenSource _cts = new();
public CancellationToken Token => _cts.Token;
public ConsoleAppCancellationTokenSource()
{
Console.CancelKeyPress += OnCancelKeyPress;
AppDomain.CurrentDomain.ProcessExit += OnProcessExit;
using var _ = _cts.Token.Register(() =>
{
AppDomain.CurrentDomain.ProcessExit -= OnProcessExit;
Console.CancelKeyPress -= OnCancelKeyPress;
});
}
private void OnCancelKeyPress(object? sender, ConsoleCancelEventArgs e)
{
// NOTE: cancel event, don't terminate the process
e.Cancel = true;
_cts.Cancel();
}
private void OnProcessExit(object? sender, EventArgs e)
{
if (_cts.IsCancellationRequested)
{
// NOTE: SIGINT (cancel key was pressed, this shouldn't ever actually hit however, as we remove the event handler upon cancellation of the `cancellationSource`)
return;
}
_cts.Cancel();
}
}
Dieser sorgt nun dafür, dass ich beim Drücken von STRG+C oder beim einem Prozessignal (z.B. mit kill
und Linux) einen CancellationToken
abbreche, anstatt die Anwendung hart zu beenden (zumindest, wenn ich es richtig verstanden habe).
Nun muss ich diesen CancellationToken
ja noch an die entsprechende Task-Verwaltung-Einheit übergeben. Das tue ich hier:
https://github.com/sandreas/tone/blob/main/tone/Commands/TagCommand.cs
mit
var tasks = packages.Select(p => Task.Run(async () =>
{
foreach (var file in p.Files)
{
// Datei laden und Tags verändern
// ...
// Änderungen speichern
track.Save()
}
}, cancellation)).ToArray();
await Task.Run(() => Task.WaitAll(tasks), cancellation);
Wie zu sehen ist, teile ich die Dateien zunächst in "Datei-Pakete" auf, die ich dann verarbeite. Jedes dieser Pakete wird als async Task
verpackt und über Task.Run
mit dem CancellationToken
wird die Verarbeitung gestartet. Nun möchte ich bei einem Cancel
natürlich nicht warten, bis ein solches Paket komplett verarbeitet ist, da es aus vielen Dateien bestehen kann, sondern das Programm möglichst direkt beenden (falls die Bearbeitung des Pakets mit vielen Dateien sehr lange dauert).
Soweit ich verstanden habe, würde das CancelationTokenSource.Cancel()
nun dazu führen, dass der angefangene async Task noch zu Ende läuft und das Programm dann terminiert. Demnach müsste ich folgendes tun, um "direkt" zu Beenden (foreach
muss vorzeitig beendet werden):
var tasks = packages.Select(p => Task.Run(async () =>
{
foreach (var file in p.Files)
{
// NEUER CODE
if(cancellation.IsCancellationRequested) {
break;
}
// NEUER CODE ENDE
// Datei laden und Tags verändern
// ...
// Änderungen speichern
track.Save()
}
}, cancellation)).ToArray();
await Task.Run(() => Task.WaitAll(tasks), cancellation);
Wäre das so richtig oder habe ich etwas übersehen / nicht verstanden?
var tasks = packages.Select(p => Task.Run(async () =>
{
foreach (var file in p.Files)
{
// Datei laden und Tags verändern
// ...
// Änderungen speichern
track.Save()
}
}, cancellation)).ToArray();
Ist kein schlauer Code. Besser wäre es das gesamte Select in <einen> Task auszulagern und nicht dutzende zu erzeugen. Ist im Endeffekt so ein Anti-Pattern bzw. Pitfall bei der Task-Kaskadierung.
Dann lässt sich der Cancellation Token auch viel einfacher anwenden.
Schlau ist der Code übrigens auch nicht, weil sich parallele Zugriffe auf das Dateisystem in den aller meisten Fällen negativ auswirken (es wird langsamer).
Wo es hilft ist, wenn die Dateizugriffe über das Netzwerk erfolgen. Auch SSDs sind bei sequential read um Längen schneller als bei parallelen Aktionen.
Ansonsten: Ja, prinzipiell verstanden.
Das CancellationToken nimmt man für das bewusste, passive Abbrechen, sodass eine Anwendung sauber sich beenden kann.
Das ist auch eine der wichtigen Vorteile von Tasks gegenüber Threads, weil man letztere nur abschießen aber nicht abbrechen kann.
- performance is a feature -
Microsoft MVP - @Website - @AzureStuttgart - github.com/BenjaminAbt - Sustainable Code
Vielen Dank für die hilfreiche Antwort. Ja, das mit dem Select steht schon recht weit oben auf meiner Liste 🙂 Der Code ist echt Mist so. Ich hatte mal geplant, den Grad Parallelisierung einstellbar zu machen (z.B. mit --jobs=4
oder sowas) um Remote-Dateisysteme schneller zu scannen, aber ich sollte wohl eher YAGNI verfolgen und es sequenziell machen.
Aber deine Bemerkung
Dann lässt sich der Cancellation Token auch viel einfacher anwenden.
hab ich noch nicht richtig verstanden... ich denke, das klügste wäre für den Anfang, statt eines Select mit Tasks einfach ein foreach zu machen, also:
public override async Task<int> ExecuteAsync(CommandContext context, TagCommandSettings settings,
CancellationToken cancellation)
{
var showDryRunMessage = false;
foreach (var p in packages)
{
if (await ProcessAudioBookPackage(p, settings, cancellation))
{
showDryRunMessage = true;
}
}
}
private async Task<bool> ProcessAudioBookPackage(AudioBookPackage p, TagCommandSettings settings,
CancellationToken cancellation)
{
var showDryRunMessage = false;
foreach (var file in p.Files)
{
if (cancellation.IsCancellationRequested)
{
break;
}
var track = new MetadataTrack(file)
{
BasePath = p.BaseDirectory?.FullName
};
var status = await _tagger.UpdateAsync(track);
if (!status)
{
_console.Error.WriteLine($"Could not update tags for file {file}: {status.Error}");
continue;
}
var currentMetadata = new MetadataTrack(file);
var diffListing = track.Diff(currentMetadata);
if (diffListing.Count == 0)
{
if (settings.DryRun || !settings.Force)
{
_console.Write(new Rule($"[green]unchanged: {Markup.Escape(track.Path ?? "")}[/]")
.LeftAligned());
continue;
}
var path = Markup.Escape(track.Path ?? "");
var message = !track.Save()
? $"[red]Force update failed: {path}[/]"
: $"[green]Forced update: {path}[/]";
_console.Write(new Rule(message)
.LeftAligned());
}
else
{
showDryRunMessage = settings.DryRun;
var diffTable = new Table().Expand();
diffTable.Title = new TableTitle($"[blue]DIFF: {Markup.Escape(track.Path ?? "")}[/]");
diffTable.AddColumn("property")
.AddColumn("current")
.AddColumn("new");
foreach (var (property, currentValue, newValue) in diffListing)
{
diffTable.AddRow(
property.ToString(),
Markup.Escape(newValue?.ToString() ?? "<null>"),
Markup.Escape(currentValue?.ToString() ?? "<null>")
);
}
if (settings.DryRun)
{
_console.Write(diffTable);
continue;
}
var path = Markup.Escape(track.Path ?? "");
var message = !track.Save()
? $"[red]Update failed: {path}[/]"
: $"[green]Updated: {path}[/]";
diffTable.Caption = new TableTitle(message);
_console.Write(diffTable);
}
}
return showDryRunMessage;
}
oder sehe ich das falsch?
hab ich noch nicht richtig verstanden...
Du hast dann nur noch einen Task, brauchst damit auch keinerlei Task.WaitAll etc...
Es gibt für das ganze auch Recommended Patterns, die so quasi heute auch noch ihre Gültigkeit haben.
Recommended patterns for CancellationToken
Idee ist immer:
ThrowIfCancellationRequested
je nach Kontext zu verwendenIsCancelled verwendet man i.d.R. wenn man aktiv auf Cancel reagieren muss (zB etwas aufräumen muss), ansonsten ThrowIfCancellationRequested
, wobei man je nachdem auf für ersteres die Exception fängt (und ggfls weiter reicht).
Die anderen Themen, die Du hast, sind Task Best Practises (zB keine Kaskadierung von Parallelität etc..).
PS: bitte keine Full Quotes.
- performance is a feature -
Microsoft MVP - @Website - @AzureStuttgart - github.com/BenjaminAbt - Sustainable Code
So, erstmal vielen Dank für die Antwort, die hat sehr geholfen.
Die anderen Themen, die Du hast, sind Task Best Practises (zB keine Kaskadierung von Parallelität etc..).
Hast du da eine gute Kurz-Referenz oder anderen Lesestoff für mich (also für "Task Best Practises")?
PS: bitte keine Full Quotes.
Geht klar... das Forum macht es einem nicht unbedingt leicht damit, aber ich achte in Zukunft drauf.
Google Suche nach "c# task best practices" 🙂Async/Await - Best Practices in Asynchronous Programming
Geht klar... das Forum macht es einem nicht unbedingt leicht damit, aber ich achte in Zukunft drauf.
99,99% bekommens hin, dass sie einfach "Antworten" drücken - oder bei "Zitieren" einfach den unrelevanten Anteil entfernen.
Besser geht immer, aber denke das ist okay. 🙂
- performance is a feature -
Microsoft MVP - @Website - @AzureStuttgart - github.com/BenjaminAbt - Sustainable Code
Firma dankt. Thema ist beantwortet.