ensure that a chromium process started by Quarto is always closed #11195
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…exitWithCleanup logic As of 2024-10 and chrome v130, there is a problem where Chrome client `close()` function won't return correctly, skipping the normal code part of closing a chromium process started. So processes were not killed. Now those browser processes are added to the same cleanup handler used by `execProcess()` logic so that in case of something going wrong, they are killed and closed at last resort when Quarto exists.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up on #11135 and #11187 as they surfaced a problem where Chromium processed where not closed after rendering.
The real issue is there
quarto-cli/src/core/cri/cri.ts
Lines 129 to 130 in c8a3e7c
where somehow
await client.close()does not return and sobrowser.close()never happens. This was confirmed by debugging interactively in VSCODE on Windows.This PR is not solving the issue which is tracked at #11196 - it is only improving current logic for 1.6 releases to make sure we do clean after ourselves in all cases, like we do with other processed.
So for that aim, this PR adds
browserDeno process to the same cleanupHandler for processes for quartoexitWithCleanup()logic. This cleanupHandler has all processes started withexecProcess()already, but we can't useexecProcess()here as we want to start the process in background, do HTML printing, then close.