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

fix: correct manifest decoding logic #202

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

gentlementlegen
Copy link
Member

@gentlementlegen gentlementlegen commented Nov 14, 2024

Refactor manifest decoding to store intermediate JSON data.

What is this change

It appears that sometimes we encounter the following error:

Error: Cannot perform I/O on behalf of a different request. I/O objects (such as streams, request/response bodies, and others) created in the context of one request handler cannot be accessed from a different request's handler. This is a limitation of Cloudflare Workers which allows us to improve overall performance. (I/O type: SpanParent)

This is specific to a Cloudflare environment and would most likely not happen on a local one. The reason is that the await result.json() is done through the function call (most likely). It is safer to store it in a variable and then passing it so that cloudflare can close that handle directly, avoiding this issue. Reference in cloudflare doc:
https://developers.cloudflare.com/workers/observability/errors/#cannot-perform-io-on-behalf-of-a-different-request

This is theoretical so I don't promise that it will solve our issues, but it's worth a try.

I asked Claude about this and this is the answer:

In JavaScript/TypeScript, awaiting a Promise immediately versus passing the Promise to another function that awaits it doesn't fundamentally change the variable lifespan. However, in the context of Cloudflare Workers and I/O operations, the timing of when you await and resolve I/O-bound objects is crucial.

Let's compare:

// Original approach
try {
  const result = await fetch(manifestUrl);
  const manifest = decodeManifest(await result.json()); // await inside decodeManifest
  _manifestCache[url] = manifest;
  return manifest;
}

// Modified approach
try {
  const result = await fetch(manifestUrl);
  const jsonData = await result.json();        // await outside
  const manifest = decodeManifest(jsonData);   // pass resolved data
  _manifestCache[url] = manifest;
  return manifest;
}

The key differences are:

  1. In the first approach, the Response object from fetch() and its associated I/O context are being passed into decodeManifest
  2. In the second approach, you're passing pure JSON data that's already been extracted from the I/O context

While both might work in regular JavaScript environments, in Cloudflare Workers, it's better to resolve I/O operations as early as possible to ensure the I/O context is properly closed and doesn't leak into other functions or get stored in ways that might cross request boundaries.

Think of it like this:

  • The Response object from fetch() is like an open file handle
  • result.json() is like reading and closing that file
  • You want to "close the file" (resolve the I/O) before passing the data around

So while the variable lifespan itself doesn't change, the timing of when you resolve the I/O context does matter in the Workers environment.

Refactor manifest decoding to store intermediate JSON data.
@gentlementlegen gentlementlegen marked this pull request as ready for review November 14, 2024 11:14
@rndquu rndquu merged commit d6b42c6 into ubiquity-os:development Nov 14, 2024
3 checks passed
@gentlementlegen gentlementlegen deleted the fix/io branch November 14, 2024 12:16
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