Skip to content
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

Obsolete Socket.UseOnlyOverlappedIO #52475

Merged
merged 6 commits into from
May 18, 2021
Merged

Obsolete Socket.UseOnlyOverlappedIO #52475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bdf8a4d
Add obsolete attribute
hrrrrustic May 7, 2021
aae702c
remove usages from tests
hrrrrustic May 7, 2021
047522c
remove test
hrrrrustic May 7, 2021
b2ca667
return tests with pragma disabling
hrrrrustic May 7, 2021
dbe097b
Apply suggestions from code review (obsolete message)
hrrrrustic May 11, 2021
96f9c98
fix comment
hrrrrustic May 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,8 @@ public Socket(System.Net.Sockets.SocketType socketType, System.Net.Sockets.Proto
[System.ObsoleteAttribute("SupportsIPv6 is obsoleted for this type, please use OSSupportsIPv6 instead. https://go.microsoft.com/fwlink/?linkid=14202")]
public static bool SupportsIPv6 { get { throw null; } }
public short Ttl { get { throw null; } set { } }
[System.ObsoleteAttribute("This property has no effect in .NET 5+ and .NET Core.")]
Copy link
Member

Choose a reason for hiding this comment

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

@terrajobst, we really need a better way to refer to this :) For anything that started off in .NET Core, are we now going to have to say .NET Core and .NET 5+?

Copy link
Member

Choose a reason for hiding this comment

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

The official guide suggests to refer to it as

".NET" (without mention of 5 or Core)

which would be terribly confusing in this case. ("This property has no effect in .NET" 🤷)

Copy link
Member

@stephentoub stephentoub May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

would be terribly confusing in this case

Right, but so is ".NET 5+ and .NET Core", as it incorrectly implies that .NET 5 isn't .NET Core.

cc: @richlander

Copy link
Member

@jkotas jkotas May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

".NET" (without mention of 5 or Core)

When you need to emphasize specific implementation (the case here), the guide suggests to use ".NET 5+ (and .NET Core)" for first reference and ".NET 5+" for subsequent references.

This guide was created just recently. I did not know about that. @antonfirsov thank you for sharing the link. There is a ton of places everywhere that use the phrase like the one used by this PR that was de-facto standard before.

Copy link
Member

Choose a reason for hiding this comment

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

the guide suggests to use ".NET 5+ (and .NET Core)"

Actually, it suggests:

.NET 5 (and .NET Core) and later versions

which is terribly verbose (at a level that makes it obscure IMO), this is why I went with .NET 5+ and .NET Core. I believe we should revisit that guide.

Right, but so is ".NET 5+ and .NET Core", as it incorrectly implies that .NET 5 isn't .NET Core.

@stephentoub although from internal (dotnet/runtime developer) perspective .NET 5 ⊂ .NET Core (being just another release), from what I see from the guide, this is not what's being communicated on the branding front.

Maybe we should open a tracking issue, to revisit the terminology? If yes where?

Copy link
Member

Choose a reason for hiding this comment

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

.NET 5 (and .NET Core) and later versions

"and later versions" is the standardized docs terminology. Yes, it is verbose and it is often abbreviated. Perhaps the guide should mention the .NET 5+ abbreviation as an option. For example, this abbrevation is often used in https://github.com/dotnet/dotnet-api-docs currently.

Maybe we should open a tracking issue, to revisit the terminology? If yes where?

The docs repo. The document that describes the terminology is there.

[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
public bool UseOnlyOverlappedIO { get { throw null; } set { } }
public System.Net.Sockets.Socket Accept() { throw null; }
public System.Threading.Tasks.Task<System.Net.Sockets.Socket> AcceptAsync() { throw null; }
Expand Down Expand Up @@ -501,6 +503,8 @@ public enum SocketInformationOptions
NonBlocking = 1,
Connected = 2,
Listening = 4,
[System.ObsoleteAttribute("This flag has no effect in .NET 5+ and .NET Core.")]
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
UseOnlyOverlappedIO = 8,
}
public enum SocketOptionLevel
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.IO;
Expand Down Expand Up @@ -415,6 +416,8 @@ public bool Blocking
// allowing calls to DuplicateAndClose() even after performing asynchronous IO.
// .NET (Core) Windows sockets are entirely IOCP-based, and the concept of "overlapped IO"
// does not exist on other platforms, therefore UseOnlyOverlappedIO is a dummy, compat-only property.
[Obsolete("This property has no effect in .NET 5+ and .NET Core.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public bool UseOnlyOverlappedIO
{
get { return false; }
Expand Down
4 changes: 4 additions & 0 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketInformationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.ComponentModel;

namespace System.Net.Sockets
{
[Flags]
Expand All @@ -11,6 +13,8 @@ public enum SocketInformationOptions
//disconnect doesn't update getpeername to return a failure.
Connected = 0x2,
Listening = 0x4,
[Obsolete("This flag has no effect in .NET 5+ and .NET Core.")]
[EditorBrowsable(EditorBrowsableState.Never)]
UseOnlyOverlappedIO = 0x8,
}
}
3 changes: 2 additions & 1 deletion src/libraries/System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,9 @@ public void Ctor_SafeHandle_BasicPropertiesPropagate_Success(AddressFamily addre
Assert.Equal(orig.ReceiveTimeout, copy.ReceiveTimeout);
Assert.Equal(orig.SendBufferSize, copy.SendBufferSize);
Assert.Equal(orig.SendTimeout, copy.SendTimeout);
#pragma warning disable 0618
Assert.Equal(orig.UseOnlyOverlappedIO, copy.UseOnlyOverlappedIO);
#pragma warning restore 0618
}

[Theory]
Expand Down Expand Up @@ -422,7 +424,6 @@ public async Task Ctor_SafeHandle_Tcp_SendReceive_Success(AddressFamily addressF
Assert.Equal(orig.SendBufferSize, client.SendBufferSize);
Assert.Equal(orig.SendTimeout, client.SendTimeout);
Assert.Equal(orig.Ttl, client.Ttl);
Assert.Equal(orig.UseOnlyOverlappedIO, client.UseOnlyOverlappedIO);

// Validate setting various properties on the new instance and seeing them roundtrip back to the original.
client.ReceiveTimeout = 42;
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketDuplicationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@ public void UseOnlyOverlappedIO_AlwaysFalse()
{
using Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);

#pragma warning disable 0618
Assert.False(s.UseOnlyOverlappedIO);
s.UseOnlyOverlappedIO = true;
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep some of the tests around, with the warning disabled around the use in tests.

We need to make sure that it still works as expected even once it is obsolete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I add pragma warning disable without diagnstic id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I can use CS0618
Okay, I'll return tests 👍

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/search?q=0618

Assert.False(s.UseOnlyOverlappedIO);
#pragma warning restore 0618
}

[PlatformSpecific(TestPlatforms.Windows)]
Expand Down