Uh oh!
There was an error while loading. Please reload this page.
make EstablishProxyTunnelAsync throw on failure status code from proxy - #50763
Conversation
…y and fix relevant test
ghost
commented
Apr 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.
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.
We probably should, yeah. I'll add some logic for this.
There was a problem hiding this comment.
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.
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.
Should we obsolete it, then?
(And why does the sync version take a CancellationToken?)
There was a problem hiding this comment.
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
commented
Apr 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