Expose ConcurrentDictionary GetOrAdd/AddOrUpdate overloads in netcoreapp1.1 by justinvp · Pull Request #13828 · dotnet/corefx

justinvp · 2016-11-19T18:55:46Z

Port #1783 from future to master. Contributes to #2869.

I cherry-picked (and slightly tweaked) the two relevant commits from the future branch:

fffd49f (fixed the conflict due to moved test file)
ee5906f (updated to use current throw null; style)

I then added a third commit that cleaned some things up, extracted tests, and exposed the APIs in netcoreapp1.1.

Adds one overload to each of GetOrAdd and AddOrUpdate. These overloads accept a generic argument that is passed through to any invocations of the supplied delegates, enabling developers to avoid delegate/closure allocations when more input is needed than just the key or existing value. For AddOrUpdate, there are two existing overloads with delegates; this only provide a new overload for the one that accepts two delegates.

#1783

justinvp · 2016-11-19T19:03:51Z

+                                        case 2:
+                                            if (s_getOrAddNetCoreApp11 != null)
+                                            {
+                                                s_getOrAddNetCoreApp11(dict, j);


This is kinda ugly... I wanted to move the tests for the new APIs out to separate test file (which seems to be the preferred way), but also wanted to keep this particular test intact. This is what I came up with to accomplish that. However, in this case, ifdefs in the tests might be cleaner, and there is some precedent with #13199, for example. @stephentoub, do you have a preference?

An alternative would be to implement the new method behavior as extension methods in the test that are compiled in only when testing the older version, and then leave this test itself clean.

justinvp · 2016-11-19T19:06:30Z

@ericstj, is there anything you think I should change in this PR to bring it in-line with what you did in #13199?

JonHanna · 2016-11-19T19:42:08Z

@@ -5,6 +5,7 @@
    <AssemblyVersion>4.0.10.0</AssemblyVersion>


Should the version not go up if public members are added?

I bumped the version (though, I'm not confident I changed it in all the right places). Tests pass locally, but CI is now failing across the board (previously was green) and I'm not sure what the issue is. Hmm.

I was as much asking because I'm not confident I bumped the right places on something I'm working on :(

Yep, I just followed your lead from your PR.

Well since the last change I tried there was copying something here, that's like Pooh and Piglet hunting Woozles and following their own footprints around a tree.

justinvp · 2016-11-20T22:15:18Z

Since this is adding new APIs, the minor version should be incremented.

What files need to be updated to bump the version to 4.1.0.0?

I tried updating the following:

src/System.Collections.Concurrent/dir.props
Changing <AssemblyVersion>4.0.14.0</AssemblyVersion> => 4.1.0.0
src/System.Collections.Concurrent/ref/System.Collections.Concurrent.csproj
Changing <AssemblyVersion>4.0.10.0</AssemblyVersion> => 4.1.0.0
src/System.Collections.Concurrent/src/System.Collections.Concurrent.csproj
Adding <AssemblyVersion>4.1.0.0</AssemblyVersion>

Tests passed for me locally, but CI was failing across the board and I couldn't find a reason for the failures. So I've reverted the version changes for now (CI is back to green).

stephentoub · 2016-11-20T23:48:53Z

Thanks, Justin. The src/test changes LGTM. @weshaggard or @joperezr could probably help with the ref/version stuff.

joperezr · 2016-11-21T17:29:49Z

@@ -5,6 +5,7 @@
    <AssemblyVersion>4.0.10.0</AssemblyVersion>


You do have to upgrade the version at least for the netcoreapp1.1 framework since you are adding API there. To do so, please update src/System.Collections.Concurrent/dir.props to have it's assembly version be 4.1.0.0. Also, you will need to keep this declaration in here, but you will need to add a Condition like:

<AssemblyVersion Condition="'$(TargetGroup)'=='netstandard1.3'">4.0.10.0</AssemblyVersion>

joperezr · 2016-11-21T17:31:20Z

+  <ItemGroup>
+    <Project Include="System.Collections.Concurrent.csproj" />
+    <Project Include="System.Collections.Concurrent.csproj">
+      <TargetGroup>netcoreapp1.1</TargetGroup>


Could you please make netcoreapp1.1 the default? that wold mean changing the .csproj to have netcoreapp1.1 as the nuget target moniker by default, and in here you will be cross compiling for netstandard1.3 instead. The reason to do this is because you want the default build of the .csproj to be the newest version of it.

joperezr · 2016-11-21T17:32:24Z

      <TestTFMs>netcoreapp1.0</TestTFMs>
    </Project>
+    <Project Include="System.Collections.Concurrent.Tests.csproj">
+      <TargetGroup>netcoreapp1.1</TargetGroup>


<TestTFMs>netcoreapp1.1</TestTFMs>

This is already the default so you don't need it.

joperezr · 2016-11-21T17:39:58Z

Looks like you are not crosscompiling the src for netcoreapp1.1 which is something that you have to fix. To do this, add a new configuration in your src/System.Collections.Concurrent.builds file for netcoreapp1.1, and then make sure that you are defining the <ContractProject> property for netstandard1.3, since you will need to pass in the targetGroup for that case.

danmoseley · 2016-11-29T21:01:13Z

@justinvp have you had a chance to address feedback?

danmoseley · 2016-11-29T21:01:20Z

oops

justinvp · 2016-12-01T03:34:33Z

@danmosemsft, sorry, I haven't had time to work through getting the version incremented. Will try to get to it in the next week or so.

justinvp · 2016-12-02T19:21:25Z

@joperezr, I made changes based on your feedback, but something still isn't right and I'm not familiar enough with the build/packaging to know what needs to be changed to address it.

Here's one of the errors at the end of msbuild.log from one of the CI builds:

D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should support API version 4.1.0.0 on .NETCoreApp,Version=v1.0/win7-x86 but D:\j\workspace\windows_nt_de---4526f5ff\packages\System.Collections.Concurrent\4.3.0\ref\netstandard1.3\System.Collections.Concurrent.dll was found to support 4.0.10.0. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]

Any ideas?

weshaggard · 2016-12-03T02:37:06Z

  <ItemGroup>
-    <ProjectReference Include="..\ref\System.Collections.Concurrent.csproj">
+    <ProjectReference Include="..\ref\System.Collections.Concurrent.builds">
      <SupportedFramework>net46;netcore50;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>


This should be updated to <SupportedFramework>netcoreapp1.1;net463;$(AllXamarinFrameworks)</SupportedFramework>

weshaggard · 2016-12-03T02:45:29Z

+  <ItemGroup>
+    <Project Include="System.Collections.Concurrent.csproj" />
+    <Project Include="System.Collections.Concurrent.csproj">
+      <TargetGroup>netstandard1.3</TargetGroup>


Not 100% sure but you many need to remove this netstandard1.3 build configuration as well. It will automatically be harvested from the last stable package.

weshaggard · 2016-12-03T02:46:12Z

  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'net46_Debug|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'net46_Release|AnyCPU'" />
+  <ItemGroup Condition="'$(TargetGroup)' == 'netstandard1.3'">


This isn't necessary as this project doesn't build a netstandard1.3 build configuration.

justinvp · 2016-12-03T22:07:21Z

Thanks, Wes. I made changes per your suggestions, but now seeing new errors in msbuild.log:

         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3 but has no compile assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3 but has no runtime assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3/win-x86 but has no compile assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3/win-x86 but has no runtime assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3/win-x64 but has no compile assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3/win-x64 but has no runtime assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3/win7-x86 but has no compile assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3/win7-x86 but has no runtime assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3/win7-x64 but has no compile assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should be supported on .NETFramework,Version=v4.6.3/win7-x64 but has no runtime assets. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent should support API version 4.1.0.0 on .NETStandard,Version=v1.7 but D:\j\workspace\windows_nt_de---4526f5ff\packages\System.Collections.Concurrent\4.3.0\ref\netstandard1.3\System.Collections.Concurrent.dll was found to support 4.0.10.0. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]
         D:\j\workspace\windows_nt_de---4526f5ff\Tools\Packaging.targets(1096,5): error : System.Collections.Concurrent has mismatched compile (4.0.10.0) and runtime (4.1.0.0) versions on .NETStandard,Version=v1.7. [D:\j\workspace\windows_nt_de---4526f5ff\src\System.Collections.Concurrent\pkg\System.Collections.Concurrent.pkgproj]

Any ideas?

ianhays · 2016-12-05T18:29:39Z

  },
  "frameworks": {
-    "netstandard1.3": {}
+    "netstandard1.3": {},


should this be netstandard1.7?

no, there is no new API for netstandard so this is fine.

ianhays · 2016-12-05T18:31:50Z

    <OutputType>Library</OutputType>
-    <NuGetTargetMoniker>.NETStandard,Version=v1.3</NuGetTargetMoniker>
+    <NuGetTargetMoniker Condition="'$(TargetGroup)'==''">.NETCoreApp,Version=v1.1</NuGetTargetMoniker>
+    <DefineConstants Condition="'$(TargetGroup)'==''">$(DefineConstants);netcoreapp11</DefineConstants>


Are these ever false? The only entry in the builds file has a null TargetGroup.

yes, if we x-compile for netstandard1.3

ianhays · 2016-12-05T18:34:16Z

+
+namespace System.Collections.Concurrent.Tests
+{
+    // Allows the ConcurrentDictionary tests to run on targets that do not have the new GetOrAdd/AddOrUpdate overloads.


Why? What's the purpose in testing a function that doesn't actually exist in that version?

It wasn't as clean to separate out the tests into a separate file in this case. Steve suggested the extension methods. #13828 (comment)

ianhays · 2016-12-05T18:34:52Z

      <TargetGroup>netstandard1.3</TargetGroup>
      <TestTFMs>netcoreapp1.0</TestTFMs>
    </Project>
+    <Project Include="System.Collections.Concurrent.Tests.csproj">


You may need to set TestTFMs.

I had that originally, but removed based on feedback from Jose as it's the default. #13828 (comment)

joperezr · 2016-12-05T19:00:47Z

  <Import Project="..\dir.props" />
  <PropertyGroup>
-    <AssemblyVersion>4.0.14.0</AssemblyVersion>
+    <AssemblyVersion>4.1.0.0</AssemblyVersion>


I talked with @ericstj and it looks like we intentionally keep AssemblyVersion as it was when we are adding API for core-only. So in this case, I guess that we have to revert this change, and that might solve your build issue.

If that's the case, should the version bump done in #13726 be reverted? cc: @JonHanna

Yes, I believe so. cc: @ericstj @stephentoub Should we log an issue to track reverting that change and bring the assembly version down again given that api changes were only for netcoreapp1.1?

joperezr · 2016-12-05T19:01:30Z

-    <ProjectReference Include="..\ref\System.Collections.Concurrent.csproj">
-      <SupportedFramework>net46;netcore50;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
+    <ProjectReference Include="..\ref\System.Collections.Concurrent.builds">
+      <SupportedFramework>netcoreapp1.1;net463;$(AllXamarinFrameworks)</SupportedFramework>


net463 [](start = 40, length = 6)

Let's remove net463 from here since we are not exposing any new API for it.

I think this should also fix your current build issue.

joperezr · 2016-12-05T19:04:17Z

  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <PropertyGroup>
-    <AssemblyVersion>4.0.10.0</AssemblyVersion>
+    <AssemblyVersion Condition="'$(TargetGroup)'=='netstandard1.3'">4.0.10.0</AssemblyVersion>


As I said on my previous comment on the dir.props file, I misguided you to update this AssemblyVersion, since it looks like we only want to update the AssemblyVersion whenever we expose new API for netstandard and not when we only expose it for netcoreapp. That means that this should go back to being: <AssemblyVersion>4.0.10.0</AssemblyVersion> That should fix your build issue.

…app1.1

justinvp · 2016-12-10T18:59:53Z

I reverted the version bump per @joperezr's feedback (#13828 (comment)). CI all green now.

joperezr

LGTM. thanks for fixing the version

justinvp · 2016-12-14T21:06:17Z

Unless there's any other feedback, this should be good to merge.

…nary Expose ConcurrentDictionary GetOrAdd/AddOrUpdate overloads in netcoreapp1.1 Commit migrated from dotnet/corefx@7d64cf1

stephentoub added 2 commits November 19, 2016 09:02

Update ref for new ConcurrentDictionary GetOrAdd/AddOrUpdate overloads

15bc9b2

#1783

dnfclas added the cla-already-signed label Nov 19, 2016

justinvp commented Nov 19, 2016

View reviewed changes

JonHanna reviewed Nov 19, 2016

View reviewed changes

justinvp force-pushed the concurrentdictionary branch 3 times, most recently from d6c0a76 to 64e42cc Compare November 20, 2016 19:26

karelz added the area-System.Collections label Nov 20, 2016

karelz assigned justinvp, ianhays and Priya91 Nov 20, 2016

joperezr reviewed Nov 21, 2016

View reviewed changes

danmoseley closed this Nov 29, 2016

danmoseley reopened this Nov 29, 2016

dnfclas added the cla-already-signed label Nov 29, 2016

This was referenced Dec 1, 2016

Add ConcurrentBag Clear APIs #14084

Closed

Add ConcurrentQueue<T> Clear method #13976

Closed

weshaggard reviewed Dec 3, 2016

View reviewed changes

ianhays approved these changes Dec 5, 2016

View reviewed changes

joperezr reviewed Dec 5, 2016

View reviewed changes

karelz modified the milestone: 1.2.0 Dec 6, 2016

Expose ConcurrentDictionary GetOrAdd/AddOrUpdate overloads in netcore…

a5c20bc

…app1.1

justinvp force-pushed the concurrentdictionary branch from 9d4fda1 to a5c20bc Compare December 10, 2016 17:54

joperezr approved these changes Dec 12, 2016

View reviewed changes

ianhays merged commit 7d64cf1 into dotnet:master Dec 14, 2016

justinvp deleted the concurrentdictionary branch December 14, 2016 21:48

justinvp mentioned this pull request Dec 30, 2016

Add async XLinq document/element loading and saving #14774

Merged

justinvp mentioned this pull request Jan 31, 2020

Revert System.Linq assembly version bump? dotnet/runtime#19666

Closed

Conversation

justinvp commented Nov 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinvp commented Nov 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinvp commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Nov 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Nov 21, 2016

Uh oh!

danmoseley commented Nov 29, 2016

Uh oh!

danmoseley commented Nov 29, 2016

Uh oh!

justinvp commented Dec 1, 2016

Uh oh!

justinvp commented Dec 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinvp commented Dec 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinvp commented Nov 20, 2016 •

edited

Loading

joperezr Nov 21, 2016 •

edited

Loading