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

chore: Add cancellation support for PdfBuilder operation #10460

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

filzrev
Copy link
Contributor

@filzrev filzrev commented Dec 14, 2024

This PR intended to fix issue #9870.

Currently when running docfx pdf command.
Ctrl+C command interruption is not accepted.
So I've added codes to manually cancel running operations.

Note

Playwright don't supports cancellationToken parameters.
So it need manually cancel operation by dispose playwright context.

Specre.Console don't support native cancellation support .
So PdfCommand/DefaultCommand is not modified. (It need to migrate to AsyncCommand in future)
https://github.com/spectreconsole/spectre.console/issues/701

What's changed in this PR

  1. Add cancellationToken parameter to PdfBuilder's public APIs
  2. Add Console.CancelKeyPress event handlers to cancel running operations
  3. Add cancellationToken parameter to following methods
    3.1. WebApplication.Start
    3.2. ParallelOptions parameter of Parallel.Foreach
  4. Call cancellationToken.ThrowIfCancellationRequested() inside loop.
  5. Rename outputPath parameter to pdfOutputPath.

Copy link

codecov bot commented Dec 14, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 79.32%. Comparing base (fe673ec) to head (253d001).
Report is 499 commits behind head on main.

Files with missing lines Patch % Lines
src/Docfx.App/PdfBuilder.cs 65.38% 8 Missing and 1 partial ⚠️
src/docfx/Models/CancellableCommandBase.cs 70.00% 3 Missing ⚠️
src/docfx/Models/PdfCommand.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10460      +/-   ##
==========================================
+ Coverage   74.31%   79.32%   +5.00%     
==========================================
  Files         536      548      +12     
  Lines       23189    23655     +466     
  Branches     4056     4064       +8     
==========================================
+ Hits        17234    18764    +1530     
+ Misses       4853     3720    -1133     
- Partials     1102     1171      +69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


task.Value = task.MaxValue;
task.StopTask();
});
});

// Wait pdfBuildTask completed or cancelled.
await Task.WhenAny(pdfBuildTask, Task.Delay(Timeout.Infinite, cts.Token));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we are not relying on the cancellation token passed to pdfBuildTask to gracefully cancel the PDF build job? This extra manual shutdown code looks suspicious to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's required because AnsiConsole.Progress().StartAsync and Playwright APIs doesn't support CancellationToken parameter.

So when operation is cancelled via CancellationToken.
It takes some seconds(about 5-10 seconds) until operations are cancelled.

So I need to use Task.WhenAny for stopping operations immediately.
And additional dipose/await code are required.
Because It can't output logs until AnsiConsole.Progress().StartAsync is completed
(Otherwise console log is outputted to wrong places)

@@ -641,4 +681,16 @@ private static StringComparison GetStringComparison()
? StringComparison.OrdinalIgnoreCase
: StringComparison.Ordinal;
}

private static CancellationTokenRegistration SubscribeCancelKeyPressEvent(CancellationTokenSource cts)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to apply to all build jobs, do we want to also enable it on the build command say in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm expecting same cancellation token should be passed to Build command also in future.

@filzrev filzrev force-pushed the fix-issue-9870 branch 2 times, most recently from 9f88341 to 43bec14 Compare December 25, 2024 22:12
@filzrev
Copy link
Contributor Author

filzrev commented Dec 25, 2024

What's changed on latest commit.

1. CancellableCommandBase.cs
Add abstract CancellableCommandBase class to support CancellationToken.
It subscribe following signal events and pass cancellation token to command.

  • SIGINT: It happens on Ctrl+C key pressed.
  • SIGQUIT: It happens on Ctrl+Break key pressed.
  • SIGTERM: It used on docker stop for example (On Windows environment it's not used)

See details: https://github.com/spectreconsole/spectre.console/issues/701

2. PdfBuilder.cs
2.1. Remove Console.CancelKeyPress event subscription. Use SIGINT signal instead.
2.2. Modify code to use cancellationToken that is passed as argument.
2.3. Modify code to use .WaitAsync(cancellationToken) instead of Task.When`.

3. DefaultCommand.cs
4. PdfCommand.cs
Modify code to use CancellableCommandBase.

TODO
Following tasks are not scope of this PR.

  • Implement Graceful Shutdown.
  • Add CancellationToken support for non-PDF relating commands (Build/Metadata)

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.

2 participants