Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.02k stars 1.19k forks source link

`return` in a generator function crashes after iterating an `@azure/core-sse` `EventMessageStream` to the end #30414

Closed jaked closed 1 week ago

jaked commented 1 month ago

Describe the bug Returning from an iteration over an EventMessageStream crashes if the entire stream has already been consumed.

To Reproduce Steps to reproduce the behavior:

  1. obtain a stream with createSseStream (e.g. from an @azure-rest/ai-inference response body)
  2. iterate the stream in a generator function (with for await (const event of stream))
  3. return from the iterator when complete (e.g. with if (event.data === "[DONE]") return)
  4. observe exception:
    TypeError: Cannot read properties of null (reading 'end')
    at Object.cancel (file:///Users/jaked/repos/githubnext/vitale/node_modules/.pnpm/@azure+core-sse@2.1.2/node_modules/@azure/core-sse/dist/esm/utils.js:54:31)
    at readableStreamDefaultControllerCancelSteps (node:internal/webstreams/readablestream:2383:39)
    at ReadableStreamDefaultController.[kCancel] (node:internal/webstreams/readablestream:1079:12)
    at ensureIsPromise (node:internal/webstreams/util:185:19)
    at readableStreamCancel (node:internal/webstreams/readablestream:1984:5)
    at readableStreamReaderGenericCancel (node:internal/webstreams/readablestream:2124:10)
    at returnSteps (node:internal/webstreams/readablestream:524:24)
    at Object.return (node:internal/webstreams/readablestream:568:11)
    at main (/Users/jaked/repos/githubnext/vitale/packages/examples/azure-ai.vnb-cellId=fUdPs5uLJxWHELOq-T1-8.tsx:40:3)
    at main.next (<anonymous>)

Expected behavior Iteration completes normally without exception.

Screenshots If applicable, add screenshots to help explain your problem.

Additional context The exception is coming from the cancel function in @azure/core-sse/utils.js, which calls stream.socket.end() when an EventMessageStream iteration returns in a generator function (since https://github.com/Azure/azure-sdk-for-js/pull/28111). But Node sets stream.socket to null when the underlying message has been consumed (I think since https://github.com/nodejs/node/pull/38505 / https://github.com/nodejs/undici/pull/834), so the call fails.

Replacing stream.socket.end() with stream.socket?.end() fixes the issue for me.

github-actions[bot] commented 1 month ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @xgithubtriage.

deyaaeldeen commented 2 weeks ago

Hi @jaked, thanks for opening this report and apologies for the delay. I agree with your analysis and proposed remedy and I opened https://github.com/Azure/azure-sdk-for-js/pull/30707. I'll let you know once we make a release. Again, thanks for your patience!

deyaaeldeen commented 2 weeks ago

@jaked @azure/core-sse@2.1.3 has been released! Please let us know if you have any other questions!

github-actions[bot] commented 2 weeks ago

Hi @jaked. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text "/unresolve" to remove the "issue-addressed" label and continue the conversation.

github-actions[bot] commented 1 week ago

Hi @jaked, since you haven’t asked that we /unresolve the issue, we’ll close this out. If you believe further discussion is needed, please add a comment /unresolve to reopen the issue.

jaked commented 6 days ago

thanks! I haven't had a chance to verify the fix but the change looks good to me.