Adds synchronous span APIs for datagram sockets.

[PJB3005]

Hope I did this right.

Part of #33418/#938

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Hope I did this right.

Part of #33418/#938

Author:	PJB3005
Assignees:	-
Labels:	area-System.Net.Sockets, new-api-needs-documentation
Milestone:	-

[geoffkizer]

Thanks, this looks good to me.

@antonfirsov should review and approve. (I think he's out today but should be back.)

To be clear -- the ReceiveMessageFrom API is already done, right? I see it listed in #33418, but I think that's incorrect, right?

[PJB3005]

#46285 did ReceiveMessageFrom.

@antonfirsov is the way I did the tests correct? I didn't test manually (since I don't know how to compile against a local runtime like this).

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

[geoffkizer]

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

Yeah, don't worry about the failing test. Test results look fine.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

Product code looks good, although I have some questions to the team regarding NetEventSource.

Can we add 0-byte send tests similarly to #51473?

[antonfirsov]

I assume the if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(...); has been omitted here, and at other places, because it's also missing from the Send(ROS) overload.

@geoffkizer (and maybe @MihaZupan) is NetEventSource an obsolete thing we are leaving behind in (functionally identical) span overloads on purpose, or is this a mistake? If it's a mistake is this something we care about and want to fix?

Here's how the same code looks like on the byte[] path:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 1326 to 1339 in 99f205c

	publicintSendTo(byte[]buffer,intoffset,intsize,SocketFlagssocketFlags,EndPointremoteEP)
	{
	ThrowIfDisposed();
	ValidateBufferArguments(buffer,offset,size);
	if(remoteEP==null)
	{
	thrownewArgumentNullException(nameof(remoteEP));
	}
	ValidateBlockingMode();
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint} size:{size} remoteEP:{remoteEP}");
	Internals.SocketAddresssocketAddress=Serialize(refremoteEP);

[geoffkizer]

NetEventSource is whatever we want for internal logging purposes. It's not intended to be public, ever. To the extent that it duplicates our public telemetry, we should remove it. But there's a lot of internal logging code that has proven useful in the past, and we don't want to just blindly rip this out.

[antonfirsov]

OK, from what you said it feels like it's not worth opening an issue for it, but we should keep it in mind. A dedupe refactor across the span/array overloads should fix this anyways.

[geoffkizer]

Yep.

A dedupe refactor across the span/array overloads should fix this anyways.

This is blocked on #46600 right?

[antonfirsov]

Yeah, with #46600 we would get rid of synchronous byte[] overloads on SocketAsyncContext, and rebuild byte[] overloads on top of Span<byte> overloads in Socket.cs.

[geoffkizer]

What else needs to happen here before we merge? @antonfirsov?

[antonfirsov]

In my #51956 (review) I recommended to add tests for the 0-byte send tests similarly to #51473. Not essential, but would be nice to have.

[PJB3005]

Ah, my bad, didn't realize that was something I should do.

[PJB3005]

I added 0-byte send tests. Do these look good @antonfirsov?

[antonfirsov]

LGTM. Test failures are unrelated.

Thanks!

[PJB3005]

Hope I did this right.

Part of #33418/#938

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Hope I did this right.

Part of #33418/#938

Author:	PJB3005
Assignees:	-
Labels:	area-System.Net.Sockets, new-api-needs-documentation
Milestone:	-

[geoffkizer]

Thanks, this looks good to me.

@antonfirsov should review and approve. (I think he's out today but should be back.)

To be clear -- the ReceiveMessageFrom API is already done, right? I see it listed in #33418, but I think that's incorrect, right?

[PJB3005]

#46285 did ReceiveMessageFrom.

@antonfirsov is the way I did the tests correct? I didn't test manually (since I don't know how to compile against a local runtime like this).

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

[geoffkizer]

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

Yeah, don't worry about the failing test. Test results look fine.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

Product code looks good, although I have some questions to the team regarding NetEventSource.

Can we add 0-byte send tests similarly to #51473?

[antonfirsov]

I assume the if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(...); has been omitted here, and at other places, because it's also missing from the Send(ROS) overload.

@geoffkizer (and maybe @MihaZupan) is NetEventSource an obsolete thing we are leaving behind in (functionally identical) span overloads on purpose, or is this a mistake? If it's a mistake is this something we care about and want to fix?

Here's how the same code looks like on the byte[] path:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 1326 to 1339 in 99f205c

	publicintSendTo(byte[]buffer,intoffset,intsize,SocketFlagssocketFlags,EndPointremoteEP)
	{
	ThrowIfDisposed();
	ValidateBufferArguments(buffer,offset,size);
	if(remoteEP==null)
	{
	thrownewArgumentNullException(nameof(remoteEP));
	}
	ValidateBlockingMode();
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint} size:{size} remoteEP:{remoteEP}");
	Internals.SocketAddresssocketAddress=Serialize(refremoteEP);

[geoffkizer]

NetEventSource is whatever we want for internal logging purposes. It's not intended to be public, ever. To the extent that it duplicates our public telemetry, we should remove it. But there's a lot of internal logging code that has proven useful in the past, and we don't want to just blindly rip this out.

[antonfirsov]

OK, from what you said it feels like it's not worth opening an issue for it, but we should keep it in mind. A dedupe refactor across the span/array overloads should fix this anyways.

[geoffkizer]

Yep.

A dedupe refactor across the span/array overloads should fix this anyways.

This is blocked on #46600 right?

[antonfirsov]

Yeah, with #46600 we would get rid of synchronous byte[] overloads on SocketAsyncContext, and rebuild byte[] overloads on top of Span<byte> overloads in Socket.cs.

[geoffkizer]

What else needs to happen here before we merge? @antonfirsov?

[antonfirsov]

In my #51956 (review) I recommended to add tests for the 0-byte send tests similarly to #51473. Not essential, but would be nice to have.

[PJB3005]

Ah, my bad, didn't realize that was something I should do.

[PJB3005]

I added 0-byte send tests. Do these look good @antonfirsov?

[antonfirsov]

LGTM. Test failures are unrelated.

Thanks!

[PJB3005]

Hope I did this right.

Part of #33418/#938

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Hope I did this right.

Part of #33418/#938

Author:	PJB3005
Assignees:	-
Labels:	area-System.Net.Sockets, new-api-needs-documentation
Milestone:	-

[geoffkizer]

Thanks, this looks good to me.

@antonfirsov should review and approve. (I think he's out today but should be back.)

To be clear -- the ReceiveMessageFrom API is already done, right? I see it listed in #33418, but I think that's incorrect, right?

[PJB3005]

#46285 did ReceiveMessageFrom.

@antonfirsov is the way I did the tests correct? I didn't test manually (since I don't know how to compile against a local runtime like this).

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

[geoffkizer]

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

Yeah, don't worry about the failing test. Test results look fine.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

Product code looks good, although I have some questions to the team regarding NetEventSource.

Can we add 0-byte send tests similarly to #51473?

[antonfirsov]

I assume the if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(...); has been omitted here, and at other places, because it's also missing from the Send(ROS) overload.

@geoffkizer (and maybe @MihaZupan) is NetEventSource an obsolete thing we are leaving behind in (functionally identical) span overloads on purpose, or is this a mistake? If it's a mistake is this something we care about and want to fix?

Here's how the same code looks like on the byte[] path:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 1326 to 1339 in 99f205c

	publicintSendTo(byte[]buffer,intoffset,intsize,SocketFlagssocketFlags,EndPointremoteEP)
	{
	ThrowIfDisposed();
	ValidateBufferArguments(buffer,offset,size);
	if(remoteEP==null)
	{
	thrownewArgumentNullException(nameof(remoteEP));
	}
	ValidateBlockingMode();
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint} size:{size} remoteEP:{remoteEP}");
	Internals.SocketAddresssocketAddress=Serialize(refremoteEP);

[geoffkizer]

NetEventSource is whatever we want for internal logging purposes. It's not intended to be public, ever. To the extent that it duplicates our public telemetry, we should remove it. But there's a lot of internal logging code that has proven useful in the past, and we don't want to just blindly rip this out.

[antonfirsov]

OK, from what you said it feels like it's not worth opening an issue for it, but we should keep it in mind. A dedupe refactor across the span/array overloads should fix this anyways.

[geoffkizer]

Yep.

A dedupe refactor across the span/array overloads should fix this anyways.

This is blocked on #46600 right?

[antonfirsov]

Yeah, with #46600 we would get rid of synchronous byte[] overloads on SocketAsyncContext, and rebuild byte[] overloads on top of Span<byte> overloads in Socket.cs.

[geoffkizer]

What else needs to happen here before we merge? @antonfirsov?

[antonfirsov]

In my #51956 (review) I recommended to add tests for the 0-byte send tests similarly to #51473. Not essential, but would be nice to have.

[PJB3005]

Ah, my bad, didn't realize that was something I should do.

[PJB3005]

I added 0-byte send tests. Do these look good @antonfirsov?

[antonfirsov]

LGTM. Test failures are unrelated.

Thanks!

[PJB3005]

Hope I did this right.

Part of #33418/#938

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Hope I did this right.

Part of #33418/#938

Author:	PJB3005
Assignees:	-
Labels:	area-System.Net.Sockets, new-api-needs-documentation
Milestone:	-

[geoffkizer]

Thanks, this looks good to me.

@antonfirsov should review and approve. (I think he's out today but should be back.)

To be clear -- the ReceiveMessageFrom API is already done, right? I see it listed in #33418, but I think that's incorrect, right?

[PJB3005]

#46285 did ReceiveMessageFrom.

@antonfirsov is the way I did the tests correct? I didn't test manually (since I don't know how to compile against a local runtime like this).

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

[geoffkizer]

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

Yeah, don't worry about the failing test. Test results look fine.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

Product code looks good, although I have some questions to the team regarding NetEventSource.

Can we add 0-byte send tests similarly to #51473?

[antonfirsov]

I assume the if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(...); has been omitted here, and at other places, because it's also missing from the Send(ROS) overload.

@geoffkizer (and maybe @MihaZupan) is NetEventSource an obsolete thing we are leaving behind in (functionally identical) span overloads on purpose, or is this a mistake? If it's a mistake is this something we care about and want to fix?

Here's how the same code looks like on the byte[] path:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 1326 to 1339 in 99f205c

	publicintSendTo(byte[]buffer,intoffset,intsize,SocketFlagssocketFlags,EndPointremoteEP)
	{
	ThrowIfDisposed();
	ValidateBufferArguments(buffer,offset,size);
	if(remoteEP==null)
	{
	thrownewArgumentNullException(nameof(remoteEP));
	}
	ValidateBlockingMode();
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint} size:{size} remoteEP:{remoteEP}");
	Internals.SocketAddresssocketAddress=Serialize(refremoteEP);

[geoffkizer]

NetEventSource is whatever we want for internal logging purposes. It's not intended to be public, ever. To the extent that it duplicates our public telemetry, we should remove it. But there's a lot of internal logging code that has proven useful in the past, and we don't want to just blindly rip this out.

[antonfirsov]

OK, from what you said it feels like it's not worth opening an issue for it, but we should keep it in mind. A dedupe refactor across the span/array overloads should fix this anyways.

[geoffkizer]

Yep.

A dedupe refactor across the span/array overloads should fix this anyways.

This is blocked on #46600 right?

[antonfirsov]

Yeah, with #46600 we would get rid of synchronous byte[] overloads on SocketAsyncContext, and rebuild byte[] overloads on top of Span<byte> overloads in Socket.cs.

[geoffkizer]

What else needs to happen here before we merge? @antonfirsov?

[antonfirsov]

In my #51956 (review) I recommended to add tests for the 0-byte send tests similarly to #51473. Not essential, but would be nice to have.

[PJB3005]

Ah, my bad, didn't realize that was something I should do.

[PJB3005]

I added 0-byte send tests. Do these look good @antonfirsov?

[antonfirsov]

LGTM. Test failures are unrelated.

Thanks!

[PJB3005]

Hope I did this right.

Part of #33418/#938

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Hope I did this right.

Part of #33418/#938

Author:	PJB3005
Assignees:	-
Labels:	area-System.Net.Sockets, new-api-needs-documentation
Milestone:	-

[geoffkizer]

Thanks, this looks good to me.

@antonfirsov should review and approve. (I think he's out today but should be back.)

To be clear -- the ReceiveMessageFrom API is already done, right? I see it listed in #33418, but I think that's incorrect, right?

[PJB3005]

#46285 did ReceiveMessageFrom.

@antonfirsov is the way I did the tests correct? I didn't test manually (since I don't know how to compile against a local runtime like this).

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

[geoffkizer]

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

Yeah, don't worry about the failing test. Test results look fine.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

Product code looks good, although I have some questions to the team regarding NetEventSource.

Can we add 0-byte send tests similarly to #51473?

[antonfirsov]

I assume the if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(...); has been omitted here, and at other places, because it's also missing from the Send(ROS) overload.

@geoffkizer (and maybe @MihaZupan) is NetEventSource an obsolete thing we are leaving behind in (functionally identical) span overloads on purpose, or is this a mistake? If it's a mistake is this something we care about and want to fix?

Here's how the same code looks like on the byte[] path:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 1326 to 1339 in 99f205c

	publicintSendTo(byte[]buffer,intoffset,intsize,SocketFlagssocketFlags,EndPointremoteEP)
	{
	ThrowIfDisposed();
	ValidateBufferArguments(buffer,offset,size);
	if(remoteEP==null)
	{
	thrownewArgumentNullException(nameof(remoteEP));
	}
	ValidateBlockingMode();
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint} size:{size} remoteEP:{remoteEP}");
	Internals.SocketAddresssocketAddress=Serialize(refremoteEP);

[geoffkizer]

NetEventSource is whatever we want for internal logging purposes. It's not intended to be public, ever. To the extent that it duplicates our public telemetry, we should remove it. But there's a lot of internal logging code that has proven useful in the past, and we don't want to just blindly rip this out.

[antonfirsov]

OK, from what you said it feels like it's not worth opening an issue for it, but we should keep it in mind. A dedupe refactor across the span/array overloads should fix this anyways.

[geoffkizer]

Yep.

A dedupe refactor across the span/array overloads should fix this anyways.

This is blocked on #46600 right?

[antonfirsov]

Yeah, with #46600 we would get rid of synchronous byte[] overloads on SocketAsyncContext, and rebuild byte[] overloads on top of Span<byte> overloads in Socket.cs.

[geoffkizer]

What else needs to happen here before we merge? @antonfirsov?

[antonfirsov]

In my #51956 (review) I recommended to add tests for the 0-byte send tests similarly to #51473. Not essential, but would be nice to have.

[PJB3005]

Ah, my bad, didn't realize that was something I should do.

[PJB3005]

I added 0-byte send tests. Do these look good @antonfirsov?

[antonfirsov]

LGTM. Test failures are unrelated.

Thanks!

[PJB3005]

Hope I did this right.

Part of #33418/#938

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Hope I did this right.

Part of #33418/#938

Author:	PJB3005
Assignees:	-
Labels:	area-System.Net.Sockets, new-api-needs-documentation
Milestone:	-

[geoffkizer]

Thanks, this looks good to me.

@antonfirsov should review and approve. (I think he's out today but should be back.)

To be clear -- the ReceiveMessageFrom API is already done, right? I see it listed in #33418, but I think that's incorrect, right?

[PJB3005]

#46285 did ReceiveMessageFrom.

@antonfirsov is the way I did the tests correct? I didn't test manually (since I don't know how to compile against a local runtime like this).

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

[geoffkizer]

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

Yeah, don't worry about the failing test. Test results look fine.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

Product code looks good, although I have some questions to the team regarding NetEventSource.

Can we add 0-byte send tests similarly to #51473?

[antonfirsov]

I assume the if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(...); has been omitted here, and at other places, because it's also missing from the Send(ROS) overload.

@geoffkizer (and maybe @MihaZupan) is NetEventSource an obsolete thing we are leaving behind in (functionally identical) span overloads on purpose, or is this a mistake? If it's a mistake is this something we care about and want to fix?

Here's how the same code looks like on the byte[] path:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 1326 to 1339 in 99f205c

	publicintSendTo(byte[]buffer,intoffset,intsize,SocketFlagssocketFlags,EndPointremoteEP)
	{
	ThrowIfDisposed();
	ValidateBufferArguments(buffer,offset,size);
	if(remoteEP==null)
	{
	thrownewArgumentNullException(nameof(remoteEP));
	}
	ValidateBlockingMode();
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint} size:{size} remoteEP:{remoteEP}");
	Internals.SocketAddresssocketAddress=Serialize(refremoteEP);

[geoffkizer]

NetEventSource is whatever we want for internal logging purposes. It's not intended to be public, ever. To the extent that it duplicates our public telemetry, we should remove it. But there's a lot of internal logging code that has proven useful in the past, and we don't want to just blindly rip this out.

[antonfirsov]

OK, from what you said it feels like it's not worth opening an issue for it, but we should keep it in mind. A dedupe refactor across the span/array overloads should fix this anyways.

[geoffkizer]

Yep.

A dedupe refactor across the span/array overloads should fix this anyways.

This is blocked on #46600 right?

[antonfirsov]

Yeah, with #46600 we would get rid of synchronous byte[] overloads on SocketAsyncContext, and rebuild byte[] overloads on top of Span<byte> overloads in Socket.cs.

[geoffkizer]

What else needs to happen here before we merge? @antonfirsov?

[antonfirsov]

In my #51956 (review) I recommended to add tests for the 0-byte send tests similarly to #51473. Not essential, but would be nice to have.

[PJB3005]

Ah, my bad, didn't realize that was something I should do.

[PJB3005]

I added 0-byte send tests. Do these look good @antonfirsov?

[antonfirsov]

LGTM. Test failures are unrelated.

Thanks!

[PJB3005]

Hope I did this right.

Part of #33418/#938

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Hope I did this right.

Part of #33418/#938

Author:	PJB3005
Assignees:	-
Labels:	area-System.Net.Sockets, new-api-needs-documentation
Milestone:	-

[geoffkizer]

Thanks, this looks good to me.

@antonfirsov should review and approve. (I think he's out today but should be back.)

To be clear -- the ReceiveMessageFrom API is already done, right? I see it listed in #33418, but I think that's incorrect, right?

[PJB3005]

#46285 did ReceiveMessageFrom.

@antonfirsov is the way I did the tests correct? I didn't test manually (since I don't know how to compile against a local runtime like this).

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

[geoffkizer]

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

Yeah, don't worry about the failing test. Test results look fine.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

Product code looks good, although I have some questions to the team regarding NetEventSource.

Can we add 0-byte send tests similarly to #51473?

[antonfirsov]

I assume the if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(...); has been omitted here, and at other places, because it's also missing from the Send(ROS) overload.

@geoffkizer (and maybe @MihaZupan) is NetEventSource an obsolete thing we are leaving behind in (functionally identical) span overloads on purpose, or is this a mistake? If it's a mistake is this something we care about and want to fix?

Here's how the same code looks like on the byte[] path:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 1326 to 1339 in 99f205c

	publicintSendTo(byte[]buffer,intoffset,intsize,SocketFlagssocketFlags,EndPointremoteEP)
	{
	ThrowIfDisposed();
	ValidateBufferArguments(buffer,offset,size);
	if(remoteEP==null)
	{
	thrownewArgumentNullException(nameof(remoteEP));
	}
	ValidateBlockingMode();
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint} size:{size} remoteEP:{remoteEP}");
	Internals.SocketAddresssocketAddress=Serialize(refremoteEP);

[geoffkizer]

NetEventSource is whatever we want for internal logging purposes. It's not intended to be public, ever. To the extent that it duplicates our public telemetry, we should remove it. But there's a lot of internal logging code that has proven useful in the past, and we don't want to just blindly rip this out.

[antonfirsov]

OK, from what you said it feels like it's not worth opening an issue for it, but we should keep it in mind. A dedupe refactor across the span/array overloads should fix this anyways.

[geoffkizer]

Yep.

A dedupe refactor across the span/array overloads should fix this anyways.

This is blocked on #46600 right?

[antonfirsov]

Yeah, with #46600 we would get rid of synchronous byte[] overloads on SocketAsyncContext, and rebuild byte[] overloads on top of Span<byte> overloads in Socket.cs.

[geoffkizer]

What else needs to happen here before we merge? @antonfirsov?

[antonfirsov]

In my #51956 (review) I recommended to add tests for the 0-byte send tests similarly to #51473. Not essential, but would be nice to have.

[PJB3005]

Ah, my bad, didn't realize that was something I should do.

[PJB3005]

I added 0-byte send tests. Do these look good @antonfirsov?

[antonfirsov]

LGTM. Test failures are unrelated.

Thanks!

[PJB3005]

Hope I did this right.

Part of #33418/#938

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Hope I did this right.

Part of #33418/#938

Author:	PJB3005
Assignees:	-
Labels:	area-System.Net.Sockets, new-api-needs-documentation
Milestone:	-

[geoffkizer]

Thanks, this looks good to me.

@antonfirsov should review and approve. (I think he's out today but should be back.)

To be clear -- the ReceiveMessageFrom API is already done, right? I see it listed in #33418, but I think that's incorrect, right?

[PJB3005]

#46285 did ReceiveMessageFrom.

@antonfirsov is the way I did the tests correct? I didn't test manually (since I don't know how to compile against a local runtime like this).

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

[geoffkizer]

Also the failing test seems to be a timeout in some HTTP API which I don't think I could have broken with these changes?

Yeah, don't worry about the failing test. Test results look fine.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

Product code looks good, although I have some questions to the team regarding NetEventSource.

Can we add 0-byte send tests similarly to #51473?

[antonfirsov]

I assume the if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(...); has been omitted here, and at other places, because it's also missing from the Send(ROS) overload.

@geoffkizer (and maybe @MihaZupan) is NetEventSource an obsolete thing we are leaving behind in (functionally identical) span overloads on purpose, or is this a mistake? If it's a mistake is this something we care about and want to fix?

Here's how the same code looks like on the byte[] path:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 1326 to 1339 in 99f205c

	publicintSendTo(byte[]buffer,intoffset,intsize,SocketFlagssocketFlags,EndPointremoteEP)
	{
	ThrowIfDisposed();
	ValidateBufferArguments(buffer,offset,size);
	if(remoteEP==null)
	{
	thrownewArgumentNullException(nameof(remoteEP));
	}
	ValidateBlockingMode();
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint} size:{size} remoteEP:{remoteEP}");
	Internals.SocketAddresssocketAddress=Serialize(refremoteEP);

[geoffkizer]

NetEventSource is whatever we want for internal logging purposes. It's not intended to be public, ever. To the extent that it duplicates our public telemetry, we should remove it. But there's a lot of internal logging code that has proven useful in the past, and we don't want to just blindly rip this out.

[antonfirsov]

OK, from what you said it feels like it's not worth opening an issue for it, but we should keep it in mind. A dedupe refactor across the span/array overloads should fix this anyways.

[geoffkizer]

Yep.

A dedupe refactor across the span/array overloads should fix this anyways.

This is blocked on #46600 right?

[antonfirsov]

Yeah, with #46600 we would get rid of synchronous byte[] overloads on SocketAsyncContext, and rebuild byte[] overloads on top of Span<byte> overloads in Socket.cs.

[geoffkizer]

What else needs to happen here before we merge? @antonfirsov?

[antonfirsov]

In my #51956 (review) I recommended to add tests for the 0-byte send tests similarly to #51473. Not essential, but would be nice to have.

[PJB3005]

Ah, my bad, didn't realize that was something I should do.

[PJB3005]

I added 0-byte send tests. Do these look good @antonfirsov?

[antonfirsov]

LGTM. Test failures are unrelated.

Thanks!