Current proposed version of System.Net.NetworkInformation 4.1 contains APM and synchronous pattern methods

[SidharthNabar]

The IPGlobalProperties abstract class has the following APM pattern/synchronous methods:

public virtual System.IAsyncResult BeginGetUnicastAddresses(System.AsyncCallback callback, object state) 
public virtual System.Net.NetworkInformation.UnicastIPAddressInformationCollection EndGetUnicastAddresses(System.IAsyncResult asyncResult)
public virtual System.Net.NetworkInformation.UnicastIPAddressInformationCollection GetUnicastAddresses()

These need to be removed in order for this contract to better align with the other APIs in .NET Core. Further, an equivalent method that follows the TPL pattern already exists in this class and should be the one that is used for this functionality:

public virtual System.Threading.Tasks.Task<System.Net.NetworkInformation.UnicastIPAddressInformationCollection> GetUnicastAddressesAsync()

[SidharthNabar]

@CIPop @mellinoe @davidsh - Please take a look confirm that these methods need to be removed.

[mellinoe]

I'm not aware of any reason that they "need" to be removed, but I have no problem with deleting them. For x-plat, I just implemented them as wrappers over the Task-based version.

[davidsh]

For new contracts such as System.Net.NetworkInformation 4.1, we don't ship with old-style APM methods or synchronous (blocking) APIs. We encourage developers to use the Task based API patterns for async work.

So, these APIs were unintentionally added to the 4.1 proposed contract and we are now correcting that as we drive toward locking down the API surface.

[CIPop]

Please take a look confirm that these methods need to be removed.

👍 Confirmed.

We should track removing these from the implementations as well.

[mellinoe]

I have a branch with them removed from x-plat:

mellinoe/corefx@dbd7fc3

[mellinoe]

Just noticed that the regular version of this method is on the chopping block as well. Actually, I'm curious as to why we have any sort of asynchronous version of this API. Is there something about the Win32 calls we are using that make this asynchronous on Windows? It doesn't seem different from any of the other statistics / properties APIs in the library, which are all exposed as synchronous methods. Is there a reason this one is special?

[SidharthNabar]

This is an abstract base class, so it doesn't have implementation of its own. However, keeping the synchronous pattern in would encourage derived classes to do a DNS lookup on a synchronous call which would be inefficient. Hence the switch to async.

[CIPop]

Is there something about the Win32 calls we are using that make this asynchronous on Windows?

Yes, this is async on Win32. See SystemIPGlobalProperties.cs.

[mellinoe]

Yes, this is async on Win32. See SystemIPGlobalProperties.cs.

That seems reason enough for me, then. Are we planning on doing this for RC1? If so, we need to get these changes in as soon as possible.

[SidharthNabar]

Yes, this is targeted for RC1. @terrajobst @weshaggard to take a look and let us know if there are any concerns from an API review standpoint.

[weshaggard]

In this small context that those APIs seem reasonable. Have we had a review of the entire NetworkInformation contract yet?