Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Capture the current AsyncContextFrame when Response is created #467

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 21, 2023

To support the following case (specifically, a response stream failing after the response has been returned and having the async context properly propagate), we have to capture the current AsyncContext when the Response object is created and ensure that we enter that context before starting the read loop for the given stream.

const als = new AsyncLocalStorage();
addEventListener('unhandledrejection', () => {
  console.log(als.getStore());
});

export default {
  fetch() {
    return als.run(123, () => {
      return new Response(new ReadableStream({
        async pull(c) {
          c.enqueue('this will cause an unhandled rejection');
        }
      }));
    });
  }
};

@harrishancock
Copy link
Collaborator

I assume the snippet is supposed to have a new Response(), like

    return als.run(123, () => {
      return new Response(new ReadableStream({
        async pull(c) {
          c.enqueue('this will cause an unhandled rejection');
        }
      }));
    });

which made me wonder, why doesn't the read loop need the context in the following case?

    return new Response(als.run(123, () => {
      return new ReadableStream({
        async pull(c) {
          c.enqueue('this will cause an unhandled rejection');
        }
      });
    }));

I guess the error is at the call to the Response constructor; it's allowed for the ReadableStream to enqueue a string, just not allowed to use that ReadableStream as a Response body, so it makes sense that the unhandledrejection would not have the context active. Does it mean that the pull(c) code is executed in an async context in the first snippet, but not the second, though?

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2023

I think the answer is: it's not exactly clear yet. Is the ReadableStream responsible for capturing the context? Is the Reader responsible for it? Should the read or pipe be executed in the context that the ReadableStream was created in or the context in which the Reader was created or the context in which read() or pipeTo/pipeThrough were called? It's just not clear what anyone would expect, to be honest. Consider the following case:

const als = new AsyncLocalStorage();

const readable = als.run(123, () => new ReadableStream({
  pull(c) {
    c.error(`boom ${als.getStore()}`);
  }
}));
const response = als.run('abc', () => new Response(readable);
const reader = als.run('xyz', () => response.body;
const read = als.run(321, () => reader.read();
const result = await als.run('???', async () => await read);

What should boom ${als.getStore()} resolve to?

This PR takes the approach that Response object captures the context and enters it before reading, using the interpretation that the read flow should use the context in which read() or pipeTo/pipeThrough are called, which means creating the ReadableStream in a specific context does not propagate it.

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2023

Updated the sample to add the new Response(...)

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2023

Thinking about it, I think we should go forward with this but there's a larger question here about how, when, and where the async context should be captured and propagated by various web standard APIs. At worst if this changes later we'll need a compat flag but this addresses the immediate need that prompted this fix.

@jasnell jasnell merged commit 26dab81 into main Mar 23, 2023
@jasnell jasnell deleted the jsnell/capture-asynccontext-on-response branch March 23, 2023 15:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants