Skip to content

Update comment on return value of NetworkStream.Read* variants #10960

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Feb 14, 2025

Fixes #10925.

Mentions that returns 0 when socket was closed.

Fixes #10925.

Mentions that returns 0 when socket was closed.
@rzikm rzikm requested a review from a team as a code owner February 14, 2025 09:20
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl

Copy link

Learn Build status updates of commit 04b0287:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Net.Sockets/NetworkStream.xml ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

The comment in the code qouted in #10925 is wrong.

When the receiver socket is closed an exception will be thrown (ODE on abortive close - or SocketException(SocketError.Shutdown) if graceful shutdown of receives has been initiated.).

0 bytes will be received when the peer has initiated a graceful termination of writes (either by calling Shutdown(Send / Both) or by calling Close(0), which means compliance with base class Stream.Read* EOF spec:

The total number of bytes read into the buffer. This can be less than the size of the buffer if that many bytes are not currently available, or zero (0) if the buffer's length is zero or the end of the stream has been reached.

When the the peer socket is aborted and sent RST, an exception SocketException(ConnectionReset) will be thrown.

So IMO the docs should be either copypaste Stream.Read content, or slightly augment it, eg.:

The total number of bytes read into the buffer. This can be less than the size of the buffer if that many bytes are not currently available, or zero (0) if the buffer's length is zero or the end of the stream has been reached, that is the peer socket has initiated graceful shutdown of writes.


Edit: My comments above are describing Socket behavior when it comes to exceptions; whatever Exception is thrown by the underlying socket is wrapped into IOException in NetworkStream. When a NetworkStream is disposed/closed directly, ObjectDisposedException is thrown.

@dotnet-policy-service dotnet-policy-service bot added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 14, 2025
@algor1
Copy link

algor1 commented Feb 14, 2025

Both suggested answers are correct, but they do not answer the user's questions about the method, but simply refer to look for information in other sources

When we come to look at the instructions, we want to see information about how the methods work. If we simply copy from the Read method, the users will find out that this method works a little differently from other streams only by making a bug in their code.

I went to the documentation to find out exactly the differences in the behavior of this method:
What does the Read method return when the data has not yet arrived?
And when does the Read method return 0. (because I already have a bug in my code)
And I did not find the answers.

0 bytes will be received when the peer has initiated a graceful termination of writes (either by calling Shutdown(Send / Both) or by calling Close(0)

Sounds much better and describes behaviour

And of course, this is just my opinion. Perhaps my point of view does not coincide with the point of view of the majority

@antonfirsov
Copy link
Member

antonfirsov commented Feb 14, 2025

0 bytes will be received when the peer has initiated a graceful termination of writes (either by calling Shutdown(Send / Both) or by calling Close(0)

Sounds much better and describes behaviour

This is true, but on the other hand, this description is .NET-specific, while the description of the peer's behavior should be worded in a platform-agnostic manner since the peer socket isn't necessarily created by .NET. At the platform level there is no Close(int timeout) operation, only close(fd), shutdown(fd, how) and SO_LINGER.

I agree we may improve our socket docs to describe various socket/TCP termination behaviors and how .NET methods map to lower-level behaviors, but IMO the <returns> doc is not the right place. If we had such docs though, we could link them from the Remarks section.

TLDR: I'm struggling to find a wording that is short, specific and platform-agnostic at the same time. (But I'm open for suggestions!)

@antonfirsov
Copy link
Member

Closing this in favor of #10983.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Sockets needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Net.Sockets.NetworkStream.Read(Byte[], Int32, Int32)
3 participants