[core] call onResponse in as*Stream methods #28111
Conversation
deyaaeldeen
requested review from
minhanh-phan,
xirzec,
joheredi,
jeremymeng and
timovv
as code owners
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
sdk/core/core-sse/src/utils.ts
Outdated
| @@ -72,7 +74,11 @@ export function ensureAsyncIterable(stream: NodeJS.ReadableStream | ReadableStre | |||
| return { | |||
| cancel: async () => { | |||
| // drain the stream | |||
| stream.resume(); | |||
| if (typeof (stream as Readable).destroy === "function"){ | |||
| (stream as Readable).destroy(); | |||
There was a problem hiding this comment.
For the node case I remember calling destroy on the IncomingMessage from http would actually destroy the entire socket and force you to renegotiate TLS in the case that you wanted to make another server request. This was the performance killer for HEAD requests before we fixed it.
Not sure why it would be causing things to hang in this case though. Maybe it's fine to destroy here since we are canceling? Hmm
There was a problem hiding this comment.
I assume you're referring to the fix in #24356. I looked into this further and here are a few interesting findings:
- pooled sockets are unrefed but for some reason they still hold up the app
- message.socket.end() seems like a more graceful way to implement cancellation here as it sends a FIN packet to the server to make it aware that it doesn't have to bother sending more data. I think we still lose the socket eventually which is not great. The other option is to let the data flow from the server and ignore it but that could have an impact on billing/perf. I am happy to revisit this again if a better design emerges.
Addressed in 08b822d.
sdk/core/core-sse/src/utils.ts
Outdated
| @@ -1,6 +1,8 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
| // Licensed under the MIT license. | |||
|
|
|||
| import { Readable } from "stream"; | |||
There was a problem hiding this comment.
style nit: I'd personally just make a type guard function to check for a "destroy" function and call it rather than pull in node typings just for a cast
There was a problem hiding this comment.
Good point. I don't like casting here so I changed the input type to be http.IncomingMessage to be honest with the client code, although core-rest-pipeline doesn't actually use it as the public type.
| @@ -6,11 +6,13 @@ | |||
|
|
|||
| /// <reference types="node" /> | |||
|
|
|||
| import { IncomingMessage } from 'http'; | |||
There was a problem hiding this comment.
| import { IncomingMessage } from 'http'; | |
| import type { IncomingMessage } from 'http'; |
nit
Port of Azure#28111 to ts-http-runtime
Packages impacted by this PR
@azure-rest/core-client and @azure/openai
Issues associated with this PR
#28093
Describe the problem that is addressed by this PR
The
onResponsecallback option is not called instreamCompletionsandstreamChatCompletions. This is because theasNodeStreamandasBrowserStreammethods in the internal client don't call them.What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
This PR addresses the issue by simply calling the onResponse call back in the
asNodeStreamandasBrowserStreammethods. I don't think we want to not call onResponse there.Are there test cases added in this PR? (If not, why?)
Yes
Provide a list of related PRs (if any)
N/A
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists