Fix: NetworkStream throwing inconsistent exceptions #40772

antonfirsov · 2020-08-13T12:42:25Z

Span<byte> overloads of NetworkStream are throwing ObjectDisposedException instead of NetworkException with ObjectDisposedException as an inner exception, when not using derived NetworkStream.

I think this is an inconsistency we should fix
/cc @scalablecory @geoffkizer @stephentoub

~~This kinda blocks creating proper test coverage in #40506~~

ghost · 2020-08-13T12:42:30Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

ghost · 2020-08-13T12:42:46Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

antonfirsov · 2020-08-13T13:59:40Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-08-13T13:59:53Z

Azure Pipelines successfully started running 1 pipeline(s).

scalablecory · 2020-08-13T14:40:20Z

What did they throw before @geoffkizer's NetworkException patch?

Naively it seems like they should be throwing ObjectDisposedException.

antonfirsov · 2020-08-13T15:16:48Z

@scalablecory it was IOException: https://github.com/dotnet/runtime/pull/40344/files#r470029203

geoffkizer · 2020-08-13T15:44:53Z

I don't quite see how my PR changed this behavior. @antonfirsov do you know?

I agree with @scalablecory that just throwing ObjectDisposedException here seems better to me. NetworkException should generally be wrapping actual network errors from Socket, not generic exceptions like ObjectDisposedException/Argument*Exception/InvalidOperationException etc. But in the PR I tried to not modify existing behavior here, even when it seems a little questionable.

geoffkizer · 2020-08-13T16:28:57Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs

+                if (errorCode != SocketError.Success)
+                {
+                    var exception = new SocketException((int)errorCode);
+                    throw NetworkErrorHelper.MapSocketException(exception);


This combined with the catch below is going to result in two NetworkExceptions, one inside the other, isn't it?

Same issue below in ReadByte

Yeah, good catch.

antonfirsov · 2020-08-13T16:30:08Z

I don't quite see how my PR changed this behavior. @antonfirsov Anton Firszov FTE do you know?

It wasn't your PR, we used to throw IOException wrapping ObjectDisposedException before.

[...] not generic exceptions like ObjectDisposedException [...]

This PR just synchronizes the behavior for one specific case (Span<T> overload with a non-derived NetworkStream) to work the same way as all other overloads.

@geoffkizer @scalablecory what you are suggesting is a breaking change, that may have consequences that we didn't yet understand.

I would only do this, if we are sure we can push that change through before Monday, otherwise harmonizing behavior is still better than leaving the code as is. We can still raise an issue for the behavior change, and implement it later if we have time (maybe even for 5.0).

antonfirsov · 2020-08-14T16:05:39Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-08-14T16:05:53Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2020-08-14T18:24:18Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs

+            }
+            catch (Exception exception) when (!(exception is OutOfMemoryException))
+            {
+                throw GetCustomNetworkException(SR.Format(SR.net_io_writefailure, exception.Message), exception);


Presumably this should be SR.net_io_readfailure, right? Similarly below.

stephentoub · 2020-09-14T17:14:35Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs

+            {
+                bytesRead = _streamSocket.Receive(buffer, SocketFlags.None, out errorCode);
+            }
+            catch (Exception exception) when (!(exception is OutOfMemoryException))


If the goal is consistency with the array-based overload, why do the catch blocks differ?

These overloads do not throw SocketException, but return SocketError as out parameter instead.

We can refactor all overloads to use either the throwing or the out variant if we really care, but I would do it in a separate PR.

I do think it would be better to be consistent here, and use the throwing versions consistently. That said, I'm fine not doing this in this PR.

…eam-exceptions # Conflicts: # src/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs

antonfirsov · 2020-11-03T15:32:03Z

I think it's worth to get this fixed for 6.0, @geoffkizer can you have another look?

src/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs

geoffkizer

Fix the duplicated lines in the test, but aside from that, LGTM

antonfirsov · 2020-11-03T21:42:38Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2020-11-03T21:42:53Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2020-11-04T12:41:57Z

The System.IO.Packaging failure is unrelated, outerloop failures are #44219:

https://dev.azure.com/dnceng/public/_build/results?buildId=874777&view=ms.vss-test-web.build-test-results-tab

A follow-up on #40772 (comment), simplifies and harmonizes the way we wrap exceptions into IOException. Having one catch block working with System.Exception seems to be enough here, no need for specific handling of SocketException.

author Stephen Toub <[email protected]> 1604601164 -0500 committer Tammy Qiu <[email protected]> 1604960878 -0500 Add stream conformance tests for TranscodingStream (dotnet#44248) * Add stream conformance tests for TranscodingStream * Special-case 0-length input buffers to TranscodingStream.Write{Async} The base implementation of Encoder.Convert doesn't like empty inputs. Regardless, if the input is empty, we can avoid a whole bunch of unnecessary work. JIT: minor inliner refactoring (dotnet#44215) Extract out the budget check logic so it can vary by inlining policy. Use this to exempt the FullPolicy from budget checking. Fix inline xml to dump the proper (full name) hash for inlinees. Update range dumper to dump ranges in hex. Remove unused QCall for WinRTSupported (dotnet#44278) ConcurrentQueueSegment allows spinning threads to sleep. (dotnet#44265) * Allow threads to sleep when ConcurrentQueue has many enqueuers/dequeuers. * Update src/libraries/System.Private.CoreLib/src/System/Collections/Concurrent/ConcurrentQueueSegment.cs Co-authored-by: Stephen Toub <[email protected]> * Apply suggestions from code review Co-authored-by: Stephen Toub <[email protected]> Co-authored-by: AMD DAYTONA EPYC <[email protected]> Co-authored-by: Stephen Toub <[email protected]> File.Exists() is not null when true (dotnet#44310) * File.Exists() is not null when true * Fix compile * Fix compile 2 [master][watchOS] Add simwatch64 support (dotnet#44303) Xcode 12.2 removed 32 bits support for watchOS simulators, this PR helps to fix xamarin/xamarin-macios#9949, we have tested the new binaries and they are working as expected ![unknown](https://user-images.githubusercontent.com/204671/98253709-64413200-1f49-11eb-9774-8c5aa416fc57.png) Co-authored-by: dalexsoto <[email protected]> Implementing support to Debugger::Break. (dotnet#44305) Set fgOptimizedFinally flag correctly (dotnet#44268) - Initialize to 0 at compiler startup - Set flag when finally cloning optimization kicks in Fixes non-deterministic generation of nop opcodes into ARM32 code Forbid `- byref cnst` -> `+ (byref -cnst)` transformation. (dotnet#44266) * Add a repro test. * Forbid the transformation for byrefs. * Update src/coreclr/src/jit/morph.cpp Co-authored-by: Andy Ayers <[email protected]> * Update src/coreclr/src/jit/morph.cpp * Fix the test return value. WriteLine is just to make sure we don't delete the value. * improve the test. avoid a possible overflow and don't waste time on printing. Co-authored-by: Andy Ayers <[email protected]> Pick libmonosgen-2.0.so from cmake install directory instead of .libs (dotnet#44291) This aligns Linux with what we already do for all the other platforms. Update SharedPerformanceCounter assert (dotnet#44333) Remove silly ToString in GetCLRInstanceString (dotnet#44335) Use targetPlatformMoniker for net5.0 and newer tfms (dotnet#43965) * Use targetPlatformMoniker for net5.0 and newer tfms * disabling analyzer, update version to 0.0, and use new format. * update the targetFramework.sdk * removing supportedOS assembly level attribute * fix linker errors and addressing feedback * making _TargetFrameworkWithoutPlatform as private [sgen] Add Ward annotations to sgen_get_total_allocated_bytes (dotnet#43833) Attempt to fix https://jenkins.mono-project.com/job/test-mono-mainline-staticanalysis/ Co-authored-by: lambdageek <[email protected]> [tests] Re-enable tests fixed by dotnet#44081 (dotnet#44212) Fixes mono/mono#15030 and fixes mono/mono#15031 and fixes mono/mono#15032 Add an implicit argument coercion check. (dotnet#43386) * Add `impCheckImplicitArgumentCoercion`. * Fix tests with type mismatch. * Try to fix VM signature. * Allow to pass byref as native int. * another fix. * Fix another IL test. [mono] Change CMakelists.txt "python" -> Python3_EXECUTABLE (dotnet#44340) Debian doesn't install a "python" binary for python3. Tweak StreamConformanceTests for cancellation (dotnet#44342) - Avoid unnecessary timers - Separate tests for precancellation, ReadAsync(byte[], ...) cancellation, and ReadAsync(Memory, ...) cancellation Use Dictionary for underlying cache of ResourceSet (dotnet#44104) Simplify catch-rethrow logic in NetworkStream (dotnet#44246) A follow-up on dotnet#40772 (comment), simplifies and harmonizes the way we wrap exceptions into IOException. Having one catch block working with System.Exception seems to be enough here, no need for specific handling of SocketException. Simple GT_NEG optimization for dotnet#13837 (dotnet#43921) * Simple arithmetic optimization with GT_NEG * Skip GT_NEG optimization when an operand is constant. Revert bitwise rotation pattern * Fixed Value Numbering assert * Cleaned up code and comments for simple GT_NEG optimization * Formatting Co-authored-by: Julie Lee <[email protected]> [master] Update dependencies from mono/linker (dotnet#44322) * Update dependencies from https://github.com/mono/linker build 20201105.1 Microsoft.NET.ILLink.Tasks From Version 6.0.0-alpha.1.20527.2 -> To Version 6.0.0-alpha.1.20555.1 * Update dependencies from https://github.com/mono/linker build 20201105.2 Microsoft.NET.ILLink.Tasks From Version 6.0.0-alpha.1.20527.2 -> To Version 6.0.0-alpha.1.20555.2 * Disable new optimization for libraries mode (it cannot work in this mode) Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Marek Safar <[email protected]> Tighten argument validation in StreamConformanceTests (dotnet#44326) Add threshold on number of files / partition in SPMI collection (dotnet#44180) * Add check for files count * Fix the OS check * decrese file limit to 1500: * misc fix * Do not upload to azure if mch files are zero size Fix ELT profiler tests (dotnet#44285) [master] Update dependencies from dotnet/arcade dotnet/llvm-project dotnet/icu (dotnet#44336) [master] Update dependencies from dotnet/arcade dotnet/llvm-project dotnet/icu - Merge branch 'master' into darc-master-2211df94-2a02-4c3c-abe1-e3534e896267 Fix Send_TimeoutResponseContent_Throws (dotnet#44356) If the client times out too quickly, the server may never have a connection to accept and will hang forever. Match CoreCLR behaviour on thread start failure (dotnet#44124) Co-authored-by: Aleksey Kliger (λgeek) <[email protected]> Add slash in Windows SoD tool build (dotnet#44359) * Add slash in Windows SoD tool build * Update SoD search path to match output dir * Fixup dotnet version * Remove merge commit headers * Disable PRs Co-authored-by: Drew Scoggins <andrew.g.scoggins@gmail> Reflect test path changes in .gitattributes; remove nonexistent files (dotnet#44371) Bootstrapping a test for R2RDump (dotnet#42150) Improve performance of Enum's generic IsDefined / GetName / GetNames (dotnet#44355) Eliminates the boxing in IsDefined/GetName/GetValues, and in GetNames avoids having to go through RuntimeType's GetEnumNames override. clarify http version test (dotnet#44379) Co-authored-by: Geoffrey Kizer <[email protected]> Update dependencies from https://github.com/mono/linker build 20201106.1 (dotnet#44367) Microsoft.NET.ILLink.Tasks From Version 6.0.0-alpha.1.20555.2 -> To Version 6.0.0-alpha.1.20556.1 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Disable RunThreadLocalTest8_Values on Mono (dotnet#44357) * Disable RunThreadLocalTest8_Values on Mono It's failing on SLES * fix typo LongProcessNamesAreSupported: make test work on distros where sleep is a symlink/script (dotnet#44299) * LongProcessNamesAreSupported: make test work on distros where sleep is a symlink/script * PR feedback Co-authored-by: Stephen Toub <[email protected]> * fix compilation Co-authored-by: Stephen Toub <[email protected]> add missing constructor overloads (dotnet#44380) Co-authored-by: Geoffrey Kizer <[email protected]> change using in ConnectCallback_UseUnixDomainSocket_Success (dotnet#44366) Clean up the samples (dotnet#44293) Update dotnet/roslyn issue link Delete stale comment about dotnet/roslyn#30797 Fix/remove TODO-NULLABLEs (dotnet#44300) * Fix/remove TODO-NULLABLEs * remove redundant ! * apply Jozkee's feedback * address feedback Update glossary (dotnet#44274) Co-authored-by: Juan Hoyos <[email protected]> Co-authored-by: Stephen Toub <[email protected]> Co-authored-by: Günther Foidl <[email protected]> Add files need for wasm executable relinking/aot to the wasm runtime pack. (dotnet#43785) Co-authored-by: Alexander Köplinger <[email protected]> Move some more UnmanagedCallersOnly tests to IL now that they're invalid C# (dotnet#43366) Fix C++ build for mono/metadata/threads.c (dotnet#44413) `throw` is a reserved keyword in C++. Disable a failing test. (dotnet#44404) Change async void System.Text.Json test to be async Task (dotnet#44418) Improve crossgen2 comparison jobs (dotnet#44119) - Fix compilation on unix platforms - Wrap use of wildcard in quotes - Print better display name into log - Fix X86 constant comparison handling - Add ability to compile specific overload via single method switches Remove some unnecessary GetTypeInfo usage (dotnet#44414) Fix MarshalTypedArrayByte and re-enable it. Re-enable TestFunctionApply

fix: NetworkStream throwing inconsistent exceptions

7d2d6aa

Dotnet-GitSync-Bot added the area-System.Net label Aug 13, 2020

antonfirsov added the area-System.Net.Sockets label Aug 13, 2020

antonfirsov removed the area-System.Net label Aug 13, 2020

antonfirsov added this to the 5.0.0 milestone Aug 13, 2020

antonfirsov added the bug label Aug 13, 2020

geoffkizer reviewed Aug 13, 2020

View reviewed changes

no more nested NetworkExceptions

3c847a5

geoffkizer reviewed Aug 14, 2020

View reviewed changes

stephentoub reviewed Sep 14, 2020

View reviewed changes

antonfirsov added 2 commits November 3, 2020 15:59

Merge branch 'exp/SendToRecvFrom_Datagram_UDP' into af/fix-NetworkStr…

875be8d

…eam-exceptions # Conflicts: # src/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs

fix exception message and update code

b90a9a8

geoffkizer reviewed Nov 3, 2020

View reviewed changes

src/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs Show resolved Hide resolved

geoffkizer approved these changes Nov 3, 2020

View reviewed changes

fix test

94e243c

antonfirsov merged commit f3d84ca into dotnet:master Nov 4, 2020

antonfirsov mentioned this pull request Nov 4, 2020

Simplify catch-rethrow logic in NetworkStream #44246

Merged

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

karelz modified the milestones: 5.0.0, 6.0.0 Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: NetworkStream throwing inconsistent exceptions #40772

Fix: NetworkStream throwing inconsistent exceptions #40772

antonfirsov commented Aug 13, 2020 •

edited

Loading

ghost commented Aug 13, 2020

ghost commented Aug 13, 2020

antonfirsov commented Aug 13, 2020

azure-pipelines bot commented Aug 13, 2020

scalablecory commented Aug 13, 2020

antonfirsov commented Aug 13, 2020

geoffkizer commented Aug 13, 2020

geoffkizer Aug 13, 2020

antonfirsov Aug 13, 2020

antonfirsov commented Aug 13, 2020

antonfirsov commented Aug 14, 2020

azure-pipelines bot commented Aug 14, 2020

geoffkizer Aug 14, 2020 •

edited

Loading

antonfirsov Nov 3, 2020

stephentoub Sep 14, 2020

antonfirsov Nov 3, 2020

antonfirsov Nov 3, 2020

geoffkizer Nov 3, 2020

antonfirsov commented Nov 3, 2020

geoffkizer left a comment

antonfirsov commented Nov 3, 2020

azure-pipelines bot commented Nov 3, 2020

antonfirsov commented Nov 4, 2020 •

edited

Loading

Fix: NetworkStream throwing inconsistent exceptions #40772

Fix: NetworkStream throwing inconsistent exceptions #40772

Conversation

antonfirsov commented Aug 13, 2020 • edited Loading

ghost commented Aug 13, 2020

ghost commented Aug 13, 2020

antonfirsov commented Aug 13, 2020

azure-pipelines bot commented Aug 13, 2020

scalablecory commented Aug 13, 2020

antonfirsov commented Aug 13, 2020

geoffkizer commented Aug 13, 2020

geoffkizer Aug 13, 2020

Choose a reason for hiding this comment

antonfirsov Aug 13, 2020

Choose a reason for hiding this comment

antonfirsov commented Aug 13, 2020

antonfirsov commented Aug 14, 2020

azure-pipelines bot commented Aug 14, 2020

geoffkizer Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

antonfirsov Nov 3, 2020

Choose a reason for hiding this comment

stephentoub Sep 14, 2020

Choose a reason for hiding this comment

antonfirsov Nov 3, 2020

Choose a reason for hiding this comment

antonfirsov Nov 3, 2020

Choose a reason for hiding this comment

geoffkizer Nov 3, 2020

Choose a reason for hiding this comment

antonfirsov commented Nov 3, 2020

geoffkizer left a comment

Choose a reason for hiding this comment

antonfirsov commented Nov 3, 2020

azure-pipelines bot commented Nov 3, 2020

antonfirsov commented Nov 4, 2020 • edited Loading

antonfirsov commented Aug 13, 2020 •

edited

Loading

geoffkizer Aug 14, 2020 •

edited

Loading

antonfirsov commented Nov 4, 2020 •

edited

Loading