Added documentation for System.Net.Sockets APIs targeted for 3.0 #2767

jozkee · 2019-07-17T01:30:05Z

List of APIs documented:

AddresFamily - Added support for new Linux protocols initial support for new network protocols corefx#37315
- ControllerAreaNetwork
- Netlink
- Packet
ProtocolFamily
- ControllerAreaNetwork
- Netlink
- Packet
NetworkStream - Added a bunch of span overloads. Also did some corrections to current documentation.
- Read(System.Span{System.Byte})
- ReadAsync(System.Memory{System.Byte},System.Threading.CancellationToken)
- ReadByte
- Write(System.ReadOnlySpan{System.Byte})
- WriteAsync(System.ReadOnlyMemory{System.Byte},System.Threading.CancellationToken)
- WriteByte(System.Byte)
SafeSocketHandle - Added Socket.SafeHandle dotnet/corefx#13470
- #ctor(System.IntPtr,System.Boolean)
Socket
- SafeHandle
SendPacketsElement - Added FileStream and Int64 OffsetLong support dotnet/corefx#25354
- #ctor(System.IO.FileStream)
- #ctor(System.IO.FileStream,System.Int64,System.Int32)
- #ctor(System.IO.FileStream,System.Int64,System.Int32,System.Boolean)
- #ctor(System.String,System.Int64,System.Int32)
- #ctor(System.String,System.Int64,System.Int32,System.Boolean)
- FileStream
- OffsetLong
- Offset (Updated)
SocketOptionName - Added support for TcpKeepAlive options dotnet/corefx#25040
I literally copied the summaries from https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-tcp-socket-options
- TcpKeepAliveInterval
- TcpKeepAliveRetryCount
- TcpKeepAliveCount

jozkee · 2019-07-17T01:35:28Z

Comments about changes to docs of existing APIs:

NetworkStream.Read:
- Removed ", or 0 if the socket was closed" from Summary (ln 1378) and Remarks (ln 1383), as far as I know, and asking @scalablecory, Read does not return zero if the socket closes.
- On Note 1, replaced IOException for InvalidOperationException since that is the correct exception type thrown when CanRead is false (ln 1386).
- Moved "There is a failure reading from the network." from ObjectDisposedException to IOException as the exception has nothing to do with the object disposal (ln 1422).
- On Remarks, removed "If the remote host shuts down the connection, and all available data has been received, this method completes immediately and return zero bytes." (ln 1383) for the same reason of bullet point 1.
- Moved "An error occurred when accessing the socket" from ArgumentOutOfRangeException to IOException (ln 1418).
- In IOException, removed "The underlying socket is closed" I think this is already covered in the message "An error occurred when accessing the socket" (ln 1418).
NetworkStream.Write:
- Moved text in Remarks section to a separate Note in order to align with Read method's Remarks. (ln 1996)
- Moved "There is a failure reading from the network." from ObjectDisposedException to IOException (ln 2031).
SendPacketElem:
- .ctor(string filepath, int32 offset, int32 count)
- .ctor(string filepath, int32 offset, int32 count, bool endOfPacket)
  Removed "The offset and count must be less than the size of the file indicated by the filepath parameter." from ArgumentOutOfRangeException since is actually not being validated on filepath overloads (ln 335 & 539); that validation is only implemented on byte[] overloads.
SendPacketElem:
- Buffer
- Filepath
  Remarks now points out that these properties default values will be null if you use a constructor other than the one that populates this property (ln 630 & 767)

jozkee · 2019-07-17T01:36:45Z

@dotnet/ncl @mairaw @rpetrusha

scalablecory · 2019-07-17T02:45:56Z

Removed ", or 0 if the socket was closed" from Summary (ln 1378) and Remarks (ln 1383), as far as I know, and asking @scalablecory, Read does not return zero if the socket closes.

As we discussed it does not return 0 when you close the socket, but it does return 0 -- this happens once you've exhausted all of the data from the socket after the other side does a shutdown(send).

rpetrusha

I've reviewed this so far up to the SendPacketsElement(FileStream, Int32, Int32) constructor. I'll submit a separate set of comments/suggestions in a separate review.

xml/System.Net.Sockets/NetworkStream.xml

rpetrusha · 2019-07-17T21:13:12Z

xml/System.Net.Sockets/NetworkStream.xml

- -or-  
-
- There is a failure reading from the network.</exception>
+ The <paramref name="size" /> parameter is greater than the length of <paramref name="buffer" /> minus the value of the <paramref name="offset" /> parameter.</exception>


Suggested change

The <paramref name="size" /> parameter is greater than the length of <paramref name="buffer" /> minus the value of the <paramref name="offset" /> parameter.</exception>

<paramref name="size" /> is greater than the length of <paramref name="buffer" /> minus the value of <paramref name="offset" />.</exception>

xml/System.Net.Sockets/SafeSocketHandle.xml

xml/System.Net.Sockets/SendPacketsElement.xml

rpetrusha · 2019-07-17T22:11:05Z

xml/System.Net.Sockets/SendPacketsElement.xml

+        <param name="fileStream">The file to be transmitted using the <see cref="M:System.Net.Sockets.Socket.SendPacketsAsync(System.Net.Sockets.SocketAsyncEventArgs)" /> method.</param>
+        <param name="offset">The offset, in bytes, from the beginning of the file to the location in the file to start sending the bytes in the file.</param>
+        <param name="count">The number of bytes to send starting from the <paramref name="offset" />. If <paramref name="count" /> is zero, the entire file is sent.</param>
+        <summary>Initializes a new instance of the <see cref="T:System.Net.Sockets.SendPacketsElement" /> class using the specified <paramref name="fileStream" />, buffer <paramref name="offset" /> and <paramref name="count" />.</summary>


Suggested change

<summary>Initializes a new instance of the <see cref="T:System.Net.Sockets.SendPacketsElement" /> class using the specified <paramref name="fileStream" />, buffer <paramref name="offset" /> and <paramref name="count" />.</summary>

<summary>Initializes a new instance of the <see cref="T:System.Net.Sockets.SendPacketsElement" /> class using the specified <range of a <see cref="T:System.IO.FileStream" /> object.</summary>

If you want me to replace

the specified filestream, buffer offset and count.

for

the specified range of a filestream object.

Shouldn't we also apply that to ALL the rest of existing overloads, including the ones taking a byte[] and a string filename path? i.e: look at summary in the above overload:

dotnet-api-docs/xml/System.Net.Sockets/SendPacketsElement.xml

Line 225 in 54c925d

<summary>Initializes a new instance of the <see cref="T:System.Net.Sockets.SendPacketsElement" /> class using the specified buffer, buffer offset, and count.</summary>

Yes, we should make the replacement elsewhere, @jozkee.

rpetrusha · 2019-07-17T22:11:42Z

xml/System.Net.Sockets/SendPacketsElement.xml

+          <format type="text/markdown"><![CDATA[  
+
+## Remarks  
+ This constructor initializes a new instance of the <xref:System.Net.Sockets.SendPacketsElement> class using a <xref:System.IO.FileStream> object. The <xref:System.Net.Sockets.SendPacketsElement> class is used with the <xref:System.Net.Sockets.SocketAsyncEventArgs.SendPacketsElements%2A?displayProperty=nameWithType> property to get or set a data buffer or file to be sent using the <xref:System.Net.Sockets.Socket.SendPacketsAsync%2A?displayProperty=nameWithType> method.  


Suggested change

This constructor initializes a new instance of the <xref:System.Net.Sockets.SendPacketsElement> class using a <xref:System.IO.FileStream> object. The <xref:System.Net.Sockets.SendPacketsElement> class is used with the <xref:System.Net.Sockets.SocketAsyncEventArgs.SendPacketsElements%2A?displayProperty=nameWithType> property to get or set a data buffer or file to be sent using the <xref:System.Net.Sockets.Socket.SendPacketsAsync%2A?displayProperty=nameWithType> method.

The <xref:System.Net.Sockets.SendPacketsElement> class is used with the <xref:System.Net.Sockets.SocketAsyncEventArgs.SendPacketsElements%2A?displayProperty=nameWithType> property to get or set a data buffer or file to be sent using the <xref:System.Net.Sockets.Socket.SendPacketsAsync%2A?displayProperty=nameWithType> method.

Same question as above, this paragraph is in every single overload.

dotnet-api-docs/xml/System.Net.Sockets/SendPacketsElement.xml

Line 230 in 54c925d

The <xref:System.Net.Sockets.SendPacketsElement.%23ctor%28System.Byte%5B%5D%2CSystem.Int32%2CSystem.Int32%29> constructor initializes a new instance of the <xref:System.Net.Sockets.SendPacketsElement> class with a byte array of data. The <xref:System.Net.Sockets.SendPacketsElement> class is used with the <xref:System.Net.Sockets.SocketAsyncEventArgs.SendPacketsElements%2A?displayProperty=nameWithType> property to get or set a data buffer or file to be sent using the <xref:System.Net.Sockets.Socket.SendPacketsAsync%2A?displayProperty=nameWithType> method.

We should try to disambiguate. For example, buffer offsets are 32-bit and 64-bit, and both are represented. The remaining param name in the constriuctor list can also be replaced.

Removed "%2A" from properties. On ReadAsync(System.Memory{System.Byte},System.Threading.CancellationToken), removed unnecessary exceptions.

jozkee · 2019-07-22T00:20:08Z

I've addresssed Ron comments. This is ready for re-review.
@rpetrusha @mairaw

xml/System.Net.Sockets/Socket.xml

rpetrusha

This looks good, @jozkee. I left three more comments, two in response to your questions and a suggested change in response to the comment from @scalablecory.

* standarizing summaries of SendPacketElements.ctors * Adding note to SafeSocketHandle, related to dotnet/corefx#39677

rpetrusha · 2019-07-26T18:36:04Z

This looks good, @jozkee. Thanks for making the additional changes. I'll merge your PR now.

…lySpan{System.Byte}) After adding documentation for System.Net.Sockets in dotnet#2767, we found out that one param went away with no proper description. This change adds the missing description to it.

…lySpan{System.Byte}) (#2897) After adding documentation for System.Net.Sockets in #2767, we found out that one param went away with no proper description. This change adds the missing description to it.

Added documentation for System.Net.Sockets APIs targeted for 3.0

54c925d

jozkee requested a review from karelz as a code owner July 17, 2019 01:30

rpetrusha suggested changes Jul 17, 2019

View reviewed changes

jozkee mentioned this pull request Jul 19, 2019

Added documentation for new System.Net APIs released in 3.0 #2663

Merged

carlossanlop requested review from davidsh, wfurt, scalablecory and geoffkizer July 19, 2019 21:25

carlossanlop assigned jozkee Jul 19, 2019

carlossanlop added 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles labels Jul 19, 2019

Addressed comments

19ac546

Removed "%2A" from properties. On ReadAsync(System.Memory{System.Byte},System.Threading.CancellationToken), removed unnecessary exceptions.

scalablecory reviewed Jul 22, 2019

View reviewed changes

xml/System.Net.Sockets/Socket.xml Show resolved Hide resolved

mairaw added the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Jul 24, 2019

rpetrusha suggested changes Jul 24, 2019

View reviewed changes

rpetrusha removed the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Jul 24, 2019

mairaw added this to the July 2019 milestone Jul 24, 2019

Addressing more commnets:

a5bfff4

* standarizing summaries of SendPacketElements.ctors * Adding note to SafeSocketHandle, related to dotnet/corefx#39677

rpetrusha approved these changes Jul 26, 2019

View reviewed changes

rpetrusha merged commit f51cd41 into dotnet:master Jul 26, 2019

jozkee mentioned this pull request Jul 31, 2019

Automatically port System.Text.Json.JsonSerializer #2880

Merged

jozkee mentioned this pull request Jul 31, 2019

Adding missing param description to NetworkStream.Write(System.ReadOnlySpan{System.Byte}) #2897

Merged

	The <paramref name="size" /> parameter is greater than the length of <paramref name="buffer" /> minus the value of the <paramref name="offset" /> parameter.</exception>
	<paramref name="size" /> is greater than the length of <paramref name="buffer" /> minus the value of <paramref name="offset" />.</exception>

	<summary>Initializes a new instance of the <see cref="T:System.Net.Sockets.SendPacketsElement" /> class using the specified <paramref name="fileStream" />, buffer <paramref name="offset" /> and <paramref name="count" />.</summary>
	<summary>Initializes a new instance of the <see cref="T:System.Net.Sockets.SendPacketsElement" /> class using the specified <range of a <see cref="T:System.IO.FileStream" /> object.</summary>

Added documentation for System.Net.Sockets APIs targeted for 3.0 #2767

Added documentation for System.Net.Sockets APIs targeted for 3.0 #2767

Uh oh!

Conversation

jozkee commented Jul 17, 2019

Uh oh!

jozkee commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jozkee commented Jul 17, 2019

Uh oh!

scalablecory commented Jul 17, 2019

Uh oh!

rpetrusha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rpetrusha Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rpetrusha Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

jozkee Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

rpetrusha Jul 24, 2019

Choose a reason for hiding this comment

Uh oh!

rpetrusha Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

jozkee Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

rpetrusha Jul 24, 2019

Choose a reason for hiding this comment

Uh oh!

jozkee commented Jul 22, 2019

Uh oh!

Uh oh!

rpetrusha left a comment

Choose a reason for hiding this comment

Uh oh!

rpetrusha commented Jul 26, 2019

Uh oh!

Uh oh!

jozkee commented Jul 17, 2019 •

edited

Loading