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

Mitigate out of free space issues while running dartdoc postprocessing. #8079

Merged
merged 4 commits into from
Sep 26, 2024

Conversation

isoos
Copy link
Collaborator

@isoos isoos commented Sep 26, 2024

@isoos isoos requested a review from jonasfj September 26, 2024 07:52
Comment on lines 100 to 105
// We are likely out of free disk space.
//
// One of the isolate has already stopped, the other may be still working.
// By deleting the dartdoc-generated content, the post-processing of the other
// isolate will stop too.
await Directory(docDir).deleteIgnoringErrors(recursive: true);
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this.

Who owns the docDir, I think we should say that it's owned by the caller to postProcessDartdoc as is currently the case.

And that postProcessDartdoc only read from docDir.

It feels messy to be sometimes deleting it here and sometimes not.
If we want to delete it here, we should always do so, and we should document it for the caller.

However, I see no reason to do that.


Here we're trying to stop the isolates from running by deleting their input files.
I think that's a bit wild. Perhaps it'd be more reasonable to:

  • (A) Not run the two thing concurrently, so we don't have to use isolates.
  • (B) Create a runInIsolate<T>(FutureOr<T> Function() computation, {Completer<void> cancel}).

Obviously, (B) would require that we create a tiny helper library for running stuff in an isolate with cancellation. And that we have tests 🤣

I think the correct approach for what you're trying to do is (B), but that the simply solution is (A). Afterall, it's fairly likely that both these operations are going to be I/O bound (maybe not post-processing).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed the docDir deletion. Agreed on (B) being the better one, but for a simple handling we may just do the cleanup.

await tmpOutDir.deleteIgnoringErrors(recursive: true);
await tmpTar.deleteIgnoringErrors();

_log.shout('Failed to run dartdoc post-processing.', e, st);
Copy link
Member

Choose a reason for hiding this comment

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

At a minimum we should exit non-zero here, right?

Copy link
Member

Choose a reason for hiding this comment

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

or rethrow?

@isoos isoos merged commit d0e6cca into dart-lang:master Sep 26, 2024
32 checks passed
@isoos isoos deleted the try-catch-postprocessing branch September 26, 2024 11:09
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