- Notifications
You must be signed in to change notification settings - Fork5.2k
make EstablishProxyTunnelAsync throw on failure status code from proxy#50763
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…y and fix relevant test
ghost commentedApr 6, 2021
Tagging subscribers to this area: @dotnet/ncl |
Uh oh!
There was an error while loading.Please reload this page.
| Stream stream= tunnelResponse.Content.ReadAsStream(cancellationToken); | ||
| return(stream,null); | ||
| return tunnelResponse.Content.ReadAsStream(cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If this ReadAsStream gets canceled (which I think would only happen if cancellation were already requested when it's called, since it's otherwise synchronous and immediate), do we need to Dispose of tunnelResponse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We probably should, yeah. I'll add some logic for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suppose technically we should be using the async overload here as well, at least when it is async...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could, but we'd changed these to use the sync one as a) we own the response content and know its behavior and b) the async one results in an unnecessary allocation. From my perspective, there should never have been a ReadAsStreamAsync... it's never async :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we obsolete it, then?
(And why does the sync version take a CancellationToken?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we obsolete it, then?
Everyone uses it. I think it'd be too painful, and in reality it's only negatively impactful if you're hyperoptimizing. I think we have bigger fish to fry... cough...headers...cough.
And why does the sync version take a CancellationToken?
Good question. I'm 99% sure I commented on it being unnecessary. I'd need to look at the code, but if memory serves technically the base implementation could actually do I/O (making the XxAsync version still relevant) if it's not overridden and delegates to the CreateContentReadStream{Async} implementation.
Co-authored-by: Stephen Toub <stoub@microsoft.com>
geoffkizer commentedApr 6, 2021
/azp run runtime-libraries-coreclr outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
…shim_mono# By Aaron Robinson (10) and others# Via GitHub* upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ...# Conflicts:#src/mono/dlls/mscordbi/CMakeLists.txt
Fixes#48679
@wfurt @dotnet/ncl