Skip to content

Unify reading from procfs to get network related information #63696

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Feb 24, 2022
Merged

Unify reading from procfs to get network related information #63696

merged 3 commits into from
Feb 24, 2022

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Jan 12, 2022 •
edited by karelz
Loading

This PR makes sure undocumented exceptions like UnauthorizedAccessException does not bubble to user code when querying network interface information.

We will throw NetworkInformationException with the message "An error was encountered while querying information from the operating system." and pass the inner exception along. It seems more appropriate than PlatformNotSupportedException (which is currently thrown when the files do not exist).

Fixes #62513

@ghost ghost assigned rzikm Jan 12, 2022
@ghost
Copy link

ghost commented Jan 12, 2022

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

Issue Details

This PR makes sure undocumented exceptions like UnauthorizedAccessException does not bubble to user code when querying network interface information.

I am still a bit unsure whether using PlatformNotSupportedException is the right direction here since we generally throw that one for platforms like Android where there is no OS API to retrieve the information, (whereas restricted container just is not allowed to read the information).

Another candidate would be NetworkInformationException with a default (0) ErrorCode.

Opinions welcome.

fixes #22927

Author: rzikm
Assignees: -
Labels:

area-Infrastructure-libraries
Milestone: -

@runfoapp runfoapp bot mentioned this pull request Jan 12, 2022
Closed
@rzikm rzikm force-pushed the 62513-procfs-reading branch from b099b55 to 70dd58d Compare January 21, 2022 08:38
@rzikm rzikm changed the title [Draft] Unify reading from procfs to get network related information Unify reading from procfs to get network related information Jan 21, 2022
@rzikm rzikm marked this pull request as ready for review January 21, 2022 09:19
@karelz karelz requested a review from a team January 21, 2022 11:26
karelz
karelz reviewed Jan 21, 2022
View reviewed changes
...tworkInformation/tests/FunctionalTests/System.Net.NetworkInformation.Functional.Tests.csproj
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<StringResourcesPath>../../src/Resources/Strings.resx</StringResourcesPath>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<IgnoreForCI Condition="'$(TargetOS)' == 'Browser'">true</IgnoreForCI>
<DefineConstants>$(DefineConstants);NETWORKINFORMATION_TEST</DefineConstants>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should name it UnitTest?

@ghost
Copy link

ghost commented Jan 21, 2022

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

Issue Details

This PR makes sure undocumented exceptions like UnauthorizedAccessException does not bubble to user code when querying network interface information.

We will throw NetworkInformationException with the message "An error was encountered while querying information from the operating system." and pass the inner exception along. It seems more appropriate than PlatformNotSupportedException (which is currently thrown when the files do not exist).

Fixes #62513

Author: rzikm
Assignees: rzikm
Labels:

area-System.Net
Milestone: -

ManickaP
ManickaP reviewed Jan 21, 2022
View reviewed changes
...Net.NetworkInformation/src/System/Net/NetworkInformation/StringParsingHelpers.Connections.cs
}

string tcp4FileContents = File.ReadAllText(tcp4ConnectionsFile);
string tcp4FileContents = ReadAllText(tcp4ConnectionsFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we getting rid of the file existence checks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason was to change the type of exception thrown to something more appropriate. PNSE is not mentioned by the docs and feels more appropriate for cases when there is no (implemented?) way of accessing the information in question.

Omitting the file existence checks will still make the code throw, but it will throw NetworkInformationException instead (which is mentioned in the docs), and its inner exception will provide more info to the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the exception type is technically breaking change. But same argument as above could be applicable here. It feels it would be better and perhaps make the helper nullable and deal with lack of the file more gracefully.

@karelz karelz added this to the 7.0.0 milestone Jan 25, 2022
wfurt
wfurt reviewed Jan 26, 2022
View reviewed changes
...Net.NetworkInformation/src/System/Net/NetworkInformation/StringParsingHelpers.Connections.cs
}

string tcp4FileContents = File.ReadAllText(tcp4ConnectionsFile);
string tcp4FileContents = ReadAllText(tcp4ConnectionsFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the exception type is technically breaking change. But same argument as above could be applicable here. It feels it would be better and perhaps make the helper nullable and deal with lack of the file more gracefully.

@wfurt
Copy link
Member

wfurt commented Jan 26, 2022

cc: @am11 and @tmds for any more thoughts.

@rzikm
Copy link
Member Author

rzikm commented Feb 2, 2022

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Feb 3, 2022

Outerloop CI failures are unrelated:

  • System.Net.Sockets.Tests.SocketAsyncEventArgsTest.AcceptAsync_WithTooSmallReceiveBuffer_Failure

@karelz karelz requested a review from scalablecory February 8, 2022 17:09
@rzikm rzikm requested a review from wfurt February 23, 2022 08:16
wfurt
wfurt approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rzikm rzikm force-pushed the 62513-procfs-reading branch from 24c5100 to 20bb873 Compare February 23, 2022 18:37
@rzikm
Copy link
Member Author

rzikm commented Feb 23, 2022

Rebasing to restart CI checks

@rzikm
Copy link
Member Author

rzikm commented Feb 24, 2022

CI failures are #65841, others unrelated.

@rzikm rzikm merged commit 6cc999f into dotnet:main Feb 24, 2022
@kunalspathak kunalspathak mentioned this pull request Mar 1, 2022
Closed
@am11 am11 mentioned this pull request Mar 9, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@karelz karelz karelz approved these changes

@danmoseley danmoseley danmoseley left review comments

@ManickaP ManickaP ManickaP left review comments

@wfurt wfurt wfurt approved these changes

@scalablecory scalablecory Awaiting requested review from scalablecory

Assignees

@rzikm rzikm

Labels
area-System.Net
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

[Linux] procfs file reads and UnauthorizedAccessException handling
5 participants
@rzikm @wfurt @karelz @danmoseley @ManickaP