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

deno - replace deprecated Deno.run with Deno.Command #8893

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

Conversation

cscheid
Copy link
Collaborator

@cscheid cscheid commented Feb 26, 2024

Closes #8891.

@cscheid
Copy link
Collaborator Author

cscheid commented Feb 26, 2024

This is a bit obnoxious. Using Deno.Command crashes command.output() with a failed assertion deep into the Deno runtime:

Stack trace:
    at assert (ext:deno_web/00_infra.js:374:11)
    at readableStreamError (ext:deno_web/06_streams.js:2479:3)
    at readableStreamCollectIntoUint8Array (ext:deno_web/06_streams.js:1039:7)
    at eventLoopTick (ext:core/01_core.js:183:11)
    at async Promise.all (index 1)
    at async ChildProcess.output (ext:runtime/40_process.js:295:49)
    at async execProcess (file:///Users/cscheid/repos/github/quarto-dev/quarto-cli/src/core/process.ts:175:20)
    at async pandocListFormats (file:///Users/cscheid/repos/github/quarto-dev/quarto-cli/src/core/pandoc/pandoc-formats.ts:17:18)

(The CI output doesn't show this stack trace, I obtained it locally)

@cscheid
Copy link
Collaborator Author

cscheid commented Feb 26, 2024

We're blocked here until we upgrade Deno, I think.

But we're blocked on upgrading Deno until we confirm that the fix for denoland/deno#22180 has landed and resolves our issues. It seems that Deno v1.40.3 fixed that problem, so we should try again.

@cscheid cscheid mentioned this pull request Feb 27, 2024
@cscheid
Copy link
Collaborator Author

cscheid commented Feb 28, 2024

This PR is currently blocked by a Deno v1.41.0 bug with the following repro:

deno_repro.ts

async function processOutput(iterator: AsyncIterable<Uint8Array>): Promise<string> {
  const decoder = new TextDecoder();
  let outputText = "";
  for await (const chunk of iterator) {
    const text = decoder.decode(chunk);
    outputText += text;
  }
  return outputText;
}

async function* iteratorFromStream(stream: ReadableStream<Uint8Array>) {
  const reader = stream.getReader();
  try {
    while (true) {
      const { done, value } = await reader.read();
      if (done) return;
      yield value;
    }
  } finally {
    reader.releaseLock();
  }
}

const command = new Deno.Command("ls", {stdout: "piped", stderr: "piped"});
const process = command.spawn();

const promises: Promise<unknown>[] = [
  processOutput(iteratorFromStream(process.stdout)),
  processOutput(iteratorFromStream(process.stderr)),
];

await Promise.all(promises);

// assertion fails here
const status = await process.output();
$ deno run --allow-all deno_repro.ts
error: Uncaught (in promise) AssertionError: Assertion failed.
const status = await process.output();
               ^
    at assert (ext:deno_web/00_infra.js:374:11)
    at readableStreamError (ext:deno_web/06_streams.js:2529:3)
    at readableStreamCollectIntoUint8Array (ext:deno_web/06_streams.js:1081:7)
    at eventLoopTick (ext:core/01_core.js:153:7)
    at async Promise.all (index 1)
    at async ChildProcess.output (ext:runtime/40_process.js:322:49)
    at async file:///Users/cscheid/Desktop/daily-log/2024/02/28/deno_repro.ts:35:16

@cscheid
Copy link
Collaborator Author

cscheid commented Feb 28, 2024

Upstream deno bug report: denoland/deno#22621

@cscheid cscheid added this to the v1.6 milestone Jun 27, 2024
@lucacasonato
Copy link

We're fixing the upstream bug now, but you can unblock yourself here by using child.status instead of child.output(). child.status just waits for the process to exit, while child.output() also tries to consume stdout and stderr until EOF, which is not necessary in your case, because you do that yourself already.

@cscheid cscheid modified the milestones: v1.6, v1.7 Nov 22, 2024
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.

Replace Deno.run with Deno.command
2 participants