Moving to ValueTuples #3973

IgorFedchenko · 2019-10-10T19:53:52Z

@Aaronontheweb After updating only few projects others won't compile because of changing signatures.

Should I continue creating separate PRs per project? In this case we could create some "value-tuples" branch in your repo and safely merge PRs until all projects will be updated and will compile, and then merge to dev with possible conflicts resolving.

Because we can not merge this PR to dev anyway until solution will compile, right?

IgorFedchenko · 2019-10-10T19:58:22Z

Some Tuples are still there in updated projects, because sometimes class types are required instead of structs. Method constraints like where T: class, or Interlocked.Exchange(ref object obj, object val) also requires reference types.
But this is not going to keep Tuples in public APIs anyway.

I am using Search with Pattern ReSharper's feature, and find/replace with regex when I need to replace types when this feature does not help. And then resolving compile errors manually.

Aaronontheweb · 2019-10-11T14:36:43Z

Some Tuples are still there in updated projects, because sometimes class types are required instead of structs. Method constraints like where T: class, or Interlocked.Exchange(ref object obj, object val) also requires reference types.

That makes sense.

Aaronontheweb · 2019-10-11T18:21:21Z

Should I continue creating separate PRs per project? In this case we could create some "value-tuples" branch in your repo and safely merge PRs until all projects will be updated and will compile, and then merge to dev with possible conflicts resolving.

I'd work backwards - do separate PRs per-project, but do higher level projects that don't have any downstream dependencies (i.e. Cluster.Sharding, Akka.Persistence.Query, Streams) first, the move onto the heavy-duty projects like Akka.Remote, Akka, Akka.Cluster...

…ix required

IgorFedchenko · 2019-10-14T20:11:49Z

All right, now this is going to be single huge PR that will replace Tuples with ValueTuples where possible.

This reverts commit e3903d3.

This reverts commit 57ba996.

This reverts commit dc45340.

IgorFedchenko · 2019-10-16T16:12:21Z

@Aaronontheweb Have a question about Akka.Streams update.
Our update is going to be breaking change anyway, that is fine. Basically, when moving from Tuple to ValueTuple, we do not allow passing null values anymore.
While in other projects this may be fine (I did not find much issues with this, or even heavy Tuple class usage at all), Akka.Streams project uses tuples a lot, and there are lots of cases when null value is used as a tuple value.

Here is an example, where all big change starts:

public static FlowWithContext<TCtx, TIn, TCtx, TOut2, TMat> Collect<TCtx, TIn, TOut, TOut2, TMat>(
	this FlowWithContext<TCtx, TIn, TCtx, TOut, TMat> flow, Func<TOut, TOut2> fn) where TOut2 : class
{
	var stage = new Collect<(TOut, TCtx), (TOut2, TCtx)?>(func: x =>
	{
		var result = fn(x.Item1);
		return ReferenceEquals(result, null) ? ((TOut2, TCtx)?)null : (result, x.Item2);
	});
	return flow.Via(Flow.FromGraph(stage));
}

The flow.Via method used here is:

public FlowWithContext<TCtxIn, TIn, TCtx2, TOut2, TMat> Via<TCtx2, TOut2, TMat2>(
	IGraph<FlowShape<(TOut, TCtxOut), (TOut2, TCtx2)>, TMat2> viaFlow) =>
	FlowWithContext.From(Flow.FromGraph(Inner).Via(viaFlow));

So it requires (TOut2, TCtx2) tuple, which can not be null, while expression ReferenceEquals(result, null) ? ((TOut2, TCtx)?)null : (result, x.Item2) may return null in some cases.

The solution of making this work without logical change is change (TOut2, TCtx2) to (TOut2, TCtx2)? (make it nullable). And this works well, but after making lots of changes like that turned out that almost all tuples, used in Akka.Streams public API, should be replaced with nullable ValueTuples.

While this can be done, I am getting lots of places where code like Select(t => t.Item1) should be replaced with Select(t => t.Value.Item1) and get Possible null reference exception warning in addition. Lots of places like that, but there is no null check in original code which makes me think that nullable tuples are not expected there.

Additionally, there is lots of code like this:

Source.From(Enumerable.Range(1, 5))
	.MapMaterializedValue<(Task<IImmutableList<int>>, Task<IImmutableList<int>>, Task<IImmutableList<int>>)?>(_ => null);

This is a part of test method, and null is used as materialization value - so I have to cast to nullable ValueTuple here, and in lots of other places. And use .Value property to get result. While obviously this source then chained with other flows and result will not produce null-tuples, compiler forces usage nullable tuples and .Value property.

Moving to nullable ValueTuple may break lots of code because of this. The other option would be to not use null as a result of Akka.Stream methods that are expected to return tuple.

So, other suggestion is - should I change null in such code to default(<tuple_type>) for example? It seems like null is actually not expected anywhere in this methods anyway, so it can be any value.

What do you think?

Aaronontheweb · 2019-10-16T16:46:13Z

@IgorFedchenko thanks for a detailed and thoughtful analysis. I'd also take a look at #3282 and see what @vasily-kirichenko did there when he worked on this problem a while back.

Moving to nullable ValueTuple may break lots of code because of this. The other option would be to not use null as a result of Akka.Stream methods that are expected to return tuple.

Nullable value tuples seem like hot garbage in this API, and generally speaking Akka.Streams shouldn't be returning null - what it sounds like we really want is a result that indicates "no results," such as NotUsed.

I think the best approach here is to move away from nullable ValueTuple output, but I say that without knowing how feasible that is - the devil is in the details of the implementation.

Can you make a list, briefly, of the areas where there will be some complex refactoring as a result of losing nullability?

Aaronontheweb · 2019-10-16T16:47:05Z

A second thought: null items inside a ValueTuple is fine, just not a null ValueTuple itself.

IgorFedchenko · 2019-10-16T19:13:26Z

@Aaronontheweb About null items inside ValueTuple - sure, the issue is with null ValueTuple itself.

In general, I suspect that any time something is returning null tuple value in stream this means that the result is not going to be used. If we need to report special "no result", Option class could be used, otherwise there is no hint that null is possible value of a tuple and everybody would be getting NullReferenceException, all the time. So, my idea is that we can put anything instead of null and this is not going to be used anyway. Only if somewhere there is a special if (tuple == null) DoOneThing() else DoAnother() check, I guess Option class would be used in that case.

This idea is somewhat approved with PR you have referenced. In this PR, some classes that I have issues with are missing (like this - but I know that FlowWithContext is recent feature, or this), but anyway whenever there was returning null in tests it is changed to default(<tuple_type>).

So I am going to use that way and keep going with non-nullable ValueTuples default values. The only place I will appreciate help with so far is this one , because I am not sure about code semantics. Seems like we are converting output elements with preserving context. But why to return null tuple value when conversion function returns null?.. Ok, I see why - this is a case I was talking about, there is a special check here that changes behavior depending on null or non-null values. @Aaronontheweb I am suggesting to change this to use Option class instead of null. Or just check for Item1.HasValue.

What do you think?

IgorFedchenko · 2019-10-16T19:16:47Z

About my last question: I have just tried to use Option in that case, and this will change signature of the method and will break more code, so change Collect class check to check only Item1.HasValue condition would be preferable.

Aaronontheweb · 2019-10-16T19:17:47Z

cc @Horusiath - see @IgorFedchenko's post above about FlowWithContext API changes. Might need some help figuring out the best way to rewrite that without using null.

IgorFedchenko · 2019-10-16T19:28:32Z

This function, that returns null if conversion result is null, is used on Collect class in single place. It is stored in _func field and used like this:

var result = WithSupervision(() => _stage._func(Grab(_stage.In)));
if (result.HasValue)
{
	if (result.Value.IsDefaultForType())
		Pull(_stage.In);
	else
		Push(_stage.Out, result.Value);
}

WithSupervision executes passed lambda, _stage._func(Grab(_stage.In)) returns null without exceptions, so result is going to be Option with value null and result.HasValue = true... All right, I see now. IsDefaultForType() method is performing check I was looking for, and this will work with ValueTuple's default value also.

Seems like I got it, but please correct me if I am wrong.

All in all, changing null to default looks like best solution for all cases I was talking about.

Aaronontheweb · 2019-10-16T19:32:00Z

@IgorFedchenko that looks like a good solution.

Aaronontheweb

LGTM - but we're blocked until #3920 is resolved.

Aaronontheweb · 2019-10-18T15:15:14Z

src/contrib/cluster/Akka.Cluster.Sharding/ClusterSharding.cs

            {
                var entityId = messageExtractor.EntityId(msg);
                var entityMessage = messageExtractor.EntityMessage(msg);
-                return Tuple.Create(entityId, entityMessage);
+                return (entityId, entityMessage);


First real piece of non-test related ValueTuple code I've seen (193 files) - this is SO MUCH nicer than what we've had historically with the traditional Tuple<,> syntax. This is going to be great.

Aaronontheweb · 2019-10-18T15:17:00Z

src/contrib/cluster/Akka.Cluster.Sharding/Shard.cs

@@ -475,7 +475,7 @@ internal static class Shards
                case Shard.PassivateIdleTick _:
                    shard.PassivateIdleEntities();
                    return true;
-                case var _ when shard.ExtractEntityId(message) != null:
+                case var _ when shard.ExtractEntityId(message).HasValue:


And yes, I love this too - no more null checks. Or at least, fewer of them.

Aaronontheweb · 2019-10-18T15:50:42Z

src/contrib/cluster/Akka.DistributedData/Replicator.cs

@@ -812,14 +812,14 @@ private DataEnvelope SetData(string key, DataEnvelope envelope)

        private Digest GetDigest(string key)
        {
-            Tuple<DataEnvelope, Digest> value;
+            (DataEnvelope, Digest) value;


A nitpick, and we definitely should not try to fit this into this pull request - we should name the ValueTuple properties, especially in public APIs, so users don't have to use the .Item1 and .Item2 properties - this will also allow us to take advantage of C#7/8 language features like tuple deconstruction.

Let's plan on doing this in separate PRs as part of an ongoing "cleanup" effort, and those can be small case-by-case PRs.

Aaronontheweb · 2019-10-18T15:57:49Z

src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt

@@ -4780,6 +4780,17 @@ namespace Akka.Util
        public static int StringHash(string s) { }
        public static int SymmetricHash<T>(System.Collections.Generic.IEnumerable<T> xs, uint seed) { }
    }
+    public struct Option<T>


Aaronontheweb · 2019-10-18T15:58:04Z

src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt

+{
+    public class static ObjectExtensions
+    {
+        public static Akka.Util.Option<T> AsOption<T>(this T obj) { }


Aaronontheweb · 2019-10-18T16:01:53Z

src/core/Akka.API.Tests/CoreAPISpec.ApproveStreams.approved.txt

        public static bool IsDefaultForType<T>(this T obj) { }
    }
-    public struct Option<T>


The other API changes in this file, all public changes to the Akka.Streams APIs, are fine since they're mostly topical.

This change removes one class from the Akka.Streams.Util namespace and moves it into Akka - totally fine with that too. There's a good possibility one day that as C# adds more functional features to the language that it too will include a first-party Option type. We will cross that bridge when we get there, but for the time being I'm happy having our own third-party solution to that problem.

Aaronontheweb · 2019-10-18T17:06:14Z

src/core/Akka.Streams/Dsl/FlowWithContextOperations.cs

            {
                var result = fn(x.Item1);
-                return ReferenceEquals(result, null) ? null : Tuple.Create(result, x.Item2);
+                return ReferenceEquals(result, null) ? default((TOut2, TCtx)) : (result, x.Item2);


I think this is probably what we're going to have to do in cases where we can't use Option or null.

Aaronontheweb · 2019-10-18T17:10:16Z

src/core/Akka.Streams/CodeGen/Dsl/UnzipWith.cs

-//     Copyright (C) 2009-2019 Lightbend Inc. <http://www.lightbend.com>
-//     Copyright (C) 2013-2019 .NET Foundation <https://github.com/akkadotnet/akka.net>
+//     Copyright (C) 2015-2016 Lightbend Inc. <http://www.lightbend.com>
+//     Copyright (C) 2013-2016 Akka.NET project <https://github.com/akkadotnet/akka.net>


Copyright is off here for some reason

Aaronontheweb · 2019-10-18T17:14:16Z

src/core/Akka.Streams/Dsl/Sink.cs

@@ -89,10 +89,10 @@ public TMat2 RunWith<TMat2>(IGraph<SourceShape<TIn>, TMat2> source, IMaterialize
        /// <param name="materializer">The materializer.</param>
        /// <returns>A tuple containing the (1) materialized value and (2) a new <see cref="Sink"/>
        ///  that can be used to consume elements from the newly materialized <see cref="Sink"/>.</returns>
-        public Tuple<TMat, Sink<TIn, NotUsed>> PreMaterialize(IMaterializer materializer)
+        public (TMat, Sink<TIn, NotUsed>) PreMaterialize(IMaterializer materializer)


Areas like these on the public APIs are where I'd really appreciate having some names for the items inside the tuple, again for deconstruction purposes. I think we can add these in separate PRs.

Aaronontheweb · 2019-10-18T17:16:29Z

src/core/Akka.Streams/Implementation/FanOut.cs


-                if (tuple == null)
+                if (!(message is ValueTuple<T, T> tuple))


Aaronontheweb · 2019-10-19T13:36:49Z

Looks like my other PR didn't totally fix the issue on Linux - have some other build failures stemming from NuGet.exe failing to download... wonder if there's some azure connectivity problems, given that we had issues with pipelines on that other PR too.

IgorFedchenko · 2019-10-22T11:18:10Z

@Aaronontheweb In error report I still see errors like this:

Routing/ConsistentHash.cs(51,17): error CS8356: Predefined type 'System.ValueTuple`2' is declared in multiple referenced assemblies: 'mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089' and 'System.ValueTuple, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' [/home/vsts/work/1/s/src/core/Akka/Akka.csproj]
Routing/ConsistentHash.cs(50,17): error CS8356: Predefined type 'System.ValueTuple`2' is declared in multiple referenced assemblies: 'mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089' and 'System.ValueTuple, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' [/home/vsts/work/1/s/src/core/Akka/Akka.csproj]

But did not noticed anything related to connectivity issues, at least at that log... Should I take some time to try to resolve this? You fix looks good, and it is not clear where does it get reference to System.ValueTuple, Version=4.0.3.0

IgorFedchenko · 2019-10-22T14:35:36Z

Hmm, can not reproduce this errors locally, when trying to build in inside mcr.microsoft.com/dotnet/core/sdk docker container all is building fine with dotnet build -f netstandard2.0. @Aaronontheweb Do you have some hints to reproduce this issue outside of CI? I guess this is some environment configuration difference

Aaronontheweb · 2019-10-22T15:05:30Z

My money is on the Mono compiler struggling with ValueTuple in some of the projects

Aaronontheweb · 2019-10-22T15:05:44Z

Don't have this issue with .NET Core compilation on Linux

IgorFedchenko · 2019-10-23T16:04:40Z

@Aaronontheweb Seems like you were right - disabling ValueTuple package reference under mono fixed the problem.

But we get another one, during Akka.MultiNodeTestRunner.Shared.Tests.csproj project build:

AsyncContext/AsyncContext.TaskQueue.cs(39,32): error CS8356: Predefined type 'System.ValueTuple`2' is declared in multiple referenced assemblies: 'mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089' and 'System.ValueTuple, Version=4.0.1.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' [/home/vsts/work/1/s/src/core/Akka.Tests.Shared.Internals/Akka.Tests.Shared.Internals.csproj]

I could not quickly find where does this project get that reference - seems like it is inside one of the referenced packages... Anyway, this does not belong to public API, so I just rolled back Tuple<T1,T2> for this class, and it did not break anything.

IgorFedchenko · 2019-10-23T16:24:09Z

@Aaronontheweb Build issues under linux are resolved now, but I had to make 2 more small fixes (rolling back to Tuple<T1, T2>) in two more places in test assemblies.

While this should not be a problem to the users, we will not be able to use ValueTuple in that projects because of mono issues. This projects do not reference System.ValueTuple directly, so I suspect that some of the referenced packages are referencing it. So this may be a problem to resolve such conflicts without modifying their sources.
But if we will detect which reference is a source of the problem, possibly we'll be able to fix that (or at least send PR for fixing that in the future). Not sure if this should be a part of this PR, so maybe better to put it in separate issue?

Aaronontheweb · 2019-10-23T17:09:59Z

But if we will detect which reference is a source of the problem, possibly we'll be able to fix that (or at least send PR for fixing that in the future). Not sure if this should be a part of this PR, so maybe better to put it in separate issue?

I'd keep a separate PR for that - this will probably be fixed as a result of changing which version of .NET Framework / .NET Core / .NET Standard we ultimately support as part of Akka.NET v1.4.0.

Aaronontheweb · 2019-10-23T17:14:11Z

@IgorFedchenko

Akka.Persistence.TestKit.Tests.CounterActorTests.CounterActor_internal_state_will_be_lost_if_underlying_

Failure on all 3 platforms

IgorFedchenko · 2019-10-23T19:51:49Z

There may be race conditions because of creating new actor with same name as terminated actor from outside of actor's parent. Committed naming fix to avoid such conflict in this test.

Aaronontheweb · 2019-10-24T14:58:08Z

Couldn't get that last check to re-run, but it was a few racy specs that have a high flip rate. We should probably disable them. Nice work @IgorFedchenko .

Updated Akka and Akka.Tests projects

897d149

IgorFedchenko added 2 commits October 10, 2019 22:59

Reverted change not related to PR

0d809aa

One more update to Akka project

563a746

IgorFedchenko changed the title ~~Moving to ValueTuples: Updated Akka and Akka.Tests projects~~ Moving to ValueTuples Oct 14, 2019

IgorFedchenko added 3 commits October 14, 2019 23:05

Made lots of replacements with resharper and regex, compiler errors f…

68c917d

…ix required

Merge branch 'dev' into value-tuples

c934a6b

Akka project compiler errors fixed

b601281

IgorFedchenko added 10 commits October 15, 2019 18:46

Akka.Remote errors fix

e233c6f

Fixed errors in Akka.Streams

dc45340

Fixed Akka.Cluster errors

e677ec4

Merge branch 'dev' into value-tuples

0d7d112

Fixed errors in Akka.Remote.Tests group and Akka.Cluster group

d5046c8

Fixed some errors in Akka.Streams.Tests

57ba996

Fixed more errors in Akka.Streams.Tests

e3903d3

Revert "Fixed more errors in Akka.Streams.Tests"

f2a2569

This reverts commit e3903d3.

Revert "Fixed some errors in Akka.Streams.Tests"

c91de8a

This reverts commit 57ba996.

Revert "Fixed errors in Akka.Streams"

e0663dc

This reverts commit dc45340.

Merge branch 'tmp' into value-tuples

a4b7ec1

Aaronontheweb approved these changes Oct 18, 2019

View reviewed changes

Aaronontheweb added this to the 1.4.0 milestone Oct 18, 2019

Merge branch 'dev' into value-tuples

8b3f7dd

Merge branch 'dev' into value-tuples

960f514

IgorFedchenko added 7 commits October 23, 2019 17:49

Do not include System.ValueTuple nuget on mono

5504a90

Merge branch 'dev' into value-tuples

1a02c16

Bringing back System.Configuration usage under mono

95084c5

csproj mono target check refactoring

037dc89

Tests assembly quick fix for mono ValueTuple support

9512ffe

csproj typo fix

bc49349

One more quick fix for mono support in Akka.Serialization.TestKit

bd0c7cf

One more fix

c44b7d3

IgorFedchenko added 2 commits October 23, 2019 22:12

No change (force CI test run)

2f1bbad

Fix possible race condition for terminated actor's name release

f44c237

Merge branch 'dev' into value-tuples

ec6e353

Aaronontheweb merged commit c09c300 into akkadotnet:dev Oct 24, 2019

This was referenced Oct 24, 2019

ValueTuple for all projects except for streams #3332

Closed

Use value tuples in Streams #3282

Closed

IgorFedchenko deleted the value-tuples branch October 24, 2019 15:16

Moving to ValueTuples #3973

Moving to ValueTuples #3973

Conversation

IgorFedchenko commented Oct 10, 2019

IgorFedchenko commented Oct 10, 2019

Aaronontheweb commented Oct 11, 2019

Aaronontheweb commented Oct 11, 2019

IgorFedchenko commented Oct 14, 2019

IgorFedchenko commented Oct 16, 2019

Aaronontheweb commented Oct 16, 2019

Aaronontheweb commented Oct 16, 2019 • edited

IgorFedchenko commented Oct 16, 2019

IgorFedchenko commented Oct 16, 2019

Aaronontheweb commented Oct 16, 2019

IgorFedchenko commented Oct 16, 2019

Aaronontheweb commented Oct 16, 2019

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb commented Oct 19, 2019

IgorFedchenko commented Oct 22, 2019 • edited

IgorFedchenko commented Oct 22, 2019

Aaronontheweb commented Oct 22, 2019

Aaronontheweb commented Oct 22, 2019

IgorFedchenko commented Oct 23, 2019 • edited

IgorFedchenko commented Oct 23, 2019

Aaronontheweb commented Oct 23, 2019

Aaronontheweb commented Oct 23, 2019

IgorFedchenko commented Oct 23, 2019

Aaronontheweb commented Oct 24, 2019

Aaronontheweb commented Oct 16, 2019 •

edited

IgorFedchenko commented Oct 22, 2019 •

edited

IgorFedchenko commented Oct 23, 2019 •

edited