Remove restrictions required by netstandard 1.x but available in 2.0 #3790

heatonmatthew · 2019-05-16T22:55:04Z

The serialization attribute, appdomains and cloneable interfaces are now supported in the netstandard2.0 libraries so can be re-enabled for dotnet core targets.

Also noticed that I included a fix in the TailChopping Router to catch AskTimeoutException and handle that in the same way as a TaskCanceledException. The associated unit test was routinely breaking without this change for me.

Building on work done in #3668

…ere missing from the 1.x library set). akkadotnet#3668

Aaronontheweb · 2019-05-16T23:38:45Z

Thanks @heatonmatthew ! Just an FYI - you might get hit with some of these #3786 in the CI system. Known issue - not your fault. Going to be dealt with by #3788 and some other work our team is doing to clean up the test suite.

I'll look over this ASAP.

Aaronontheweb

Had a couple of questions, but overall looks really good

Aaronontheweb · 2019-05-16T23:39:28Z

build.fsx

-let outputBinariesNet45 = outputBinaries @@ "net45"
-let outputBinariesNetStandard = outputBinaries @@ "netstandard1.6"
+let outputBinariesNet45 = outputBinaries @@ "net452"
+let outputBinariesNetStandard = outputBinaries @@ "netstandard2.0"


build.fsx changes look fine.

Aaronontheweb · 2019-05-16T23:41:02Z

src/contrib/cluster/Akka.Cluster.Sharding.Tests.MultiNode/AsyncWriteProxyEx.cs

@@ -43,7 +43,6 @@ public AsyncReplayTimeoutException(string message)
        {
        }

-#if SERIALIZATION


I can't tell you how thrilled I am to get rid of this crap @heatonmatthew - so far, according to the CI system, the compiler and the test runtimes seem pretty happy with these changes so I don't have much to add on each individual change here.

Aaronontheweb · 2019-05-16T23:42:07Z

src/core/Akka.MultiNodeTestRunner/Discovery.cs

@@ -106,20 +98,11 @@ internal static ConstructorInfo FindConfigConstructor(Type configUser)
            var current = configUser;
            while (current != null)
            {
-
-#if CORECLR
-                var ctorWithConfig = current


I think this is probably fine, but I'll need to see the feedback from the MNTR for certain

Aaronontheweb · 2019-05-16T23:42:44Z

src/core/Akka.Remote.Tests/Transport/DotNettySslSupportSpec.cs

@@ -194,12 +194,8 @@ private void InstallCert()
            }
            finally
            {
-#if NET452 //netstandard 1.6 doesn't have close on store
                store.Close();


Aaronontheweb · 2019-05-16T23:43:47Z

src/core/Akka.Streams.TestKit/TestSubscriber.cs

@@ -34,7 +34,7 @@ public OnSubscribe(ISubscription subscription)
            public override string ToString() => $"TestSubscriber.OnSubscribe({Subscription})";
        }

-        public struct OnNext<T> : ISubscriberEvent
+        public struct OnNext<T> : ISubscriberEvent, IEquatable<OnNext<T>>


What's the purpose of this change?

I accidentally brought this across from a branch in which the FluentAssertions library had been upgraded. There were one or two tests that were failing in the assertion because there was no default comparer that worked correctly for these messages.

Probably best for me to remove this change for the time being?

I don't think it's hurting anything, so no - you can leave it. Just wanted to make sure I wasn't overlooking something more gruesome (i.e. the entire Akka.Streams test suite exploded because equality was broken by some inexplicable platform change and this is the workaround.) But based on your explanation this sounds innocent enough. We'll keep it in.

Aaronontheweb · 2019-05-16T23:44:41Z

src/core/Akka.Tests/Actor/Dispatch/Bug2640Spec.cs

+                // test which intermittently can fail because all messages are actually serviced 
+                // by a single thread from the pool. Happens under load, presumably when the processor
+                // finds it more efficient to keep thread locality of the execution.
+                Thread.Sleep(1);


Thanks for the explanation - makes sense.

Aaronontheweb · 2019-05-16T23:45:10Z

src/core/Akka/Dispatch/AbstractDispatcher.cs

@@ -224,7 +222,7 @@ internal sealed class ThreadPoolExecutorServiceFactory : ExecutorServiceConfigur
        /// <returns>TBD</returns>
        public override ExecutorService Produce(string id)
        {
-#if APPDOMAIN
+#if UNSAFE_THREADING


Aaronontheweb · 2019-05-16T23:45:26Z

src/core/Akka/Routing/TailChopping.cs

+                    }
+                    catch (AskTimeoutException)
+                    {
+                        completion.TrySetResult(


Ah, is this your fix from your other PR @heatonmatthew ?

Yes it is. Happy to remove it if you want them separated?

Nope, it's fine to keep it in. Just wanted to make sure I understood what it was.

Aaronontheweb

@heatonmatthew answered my questions - looks good to me. Ready for merge once CI completes.

heatonmatthew · 2019-05-17T07:31:23Z

@Aaronontheweb I've been doing some testing of the Akka.Persistence.SqlServer project building against a version of this branch. I've found an assumption in the Akka.Persistence.TCK.Journal.JournalSpec regarding JSON serialization that are not universally true that I think are worth mentioning.

The test is attempting to force JSON serialization to fail by implementing a test event as shown below:

        /// <summary>
        /// JSON serializer should fail on this
        /// </summary>
        private class NotSerializableEvent : ISerializable
        {
            public NotSerializableEvent(bool foo) { }

            public void GetObjectData(SerializationInfo info, StreamingContext context)
            {
                throw new NotImplementedException();
            }
        }

Now that we are running this against a netcoreapp2.1 target, it fails consistently. Thinking it through, I'm a little dubious that the comment about the class failing JSON serialization makes complete sense (because JSON serialization is different to .NET runtime serialization). After playing around with this a bit, I think that the Newtonsoft library author actually agrees with me because the behavior differs between versions as follows:

Runtime	Newtonsoft 9.x	Newtonsoft 12.x
net452	Exception	OK
netcore2.1	OK	OK

I can't think of a scenario in which anyone would be relying on this behaviour in a real non-test application (i.e. deliberately trying to break JSON serialization) but I thought it's worth a broader ponder.

Now the tests can be easily fixed by changing the test class to the following.

        /// <summary>
        /// JSON serializer should fail on this
        /// </summary>
        private class NotSerializableEvent : ISerializable
        {
            public NotSerializableEvent(bool foo) { }

            public string Data => throw new NotImplementedException();

            public void GetObjectData(SerializationInfo info, StreamingContext context)
            {
                throw new NotImplementedException();
            }
        }

Do you have any strong opinion about whether this is likely to be a broader concern?

Aaronontheweb · 2019-06-04T14:36:16Z

I need to look at this again once I'm finished with the changes in #3744

Aaronontheweb · 2019-06-26T21:06:31Z

@heatonmatthew regarding your earlier comment, can you link me to the test classes where this should fail? Trying to understand your comment from earlier.

heatonmatthew · 2019-07-15T22:18:53Z

@Aaronontheweb sorry for the slow response. The test in question is Journal_optionally_may_reject_non_serializable_events.

In the master branch, the entire test including the NotSerializableEvent is not built for Dotnet Core because of a #if !CORECLR block. It was probably switched out for netcore originally because serialization wasn't supported at that time.

I removed the compiler directive in my branch which is probably why I'm seeing the error but the mainline tests aren't yet.

Aaronontheweb · 2019-07-16T13:14:22Z

Got it, thanks @heatonmatthew - I'll see about getting this merged in ASAP. Going to re-run CI now with many of the other changes we've made.

Aaronontheweb · 2019-07-16T13:29:22Z

For some reason our incremental build system isn't detecting any changes here - wonder if that has to do with build.fsx's changes. Let me investigate that.

Arkatufus · 2021-04-16T20:24:55Z

Everything should pass now

Arkatufus · 2021-04-16T20:26:38Z

src/core/Akka.Remote/Serialization/ExceptionSupport.cs

-                serializationInfo.AddValue(field.Key, _wrappedPayloadSupport.PayloadFrom(field.Value));
+                var payload = _wrappedPayloadSupport.PayloadFrom(field.Value);
+                if (payload is ExceptionData exception)
+                    payload = ExceptionFromProto(exception);


Old code would fail if it encounters an Exception property inside an Exception, have to make sure that the serializer recurse through these

Arkatufus · 2021-04-16T20:27:30Z

src/core/Akka.Remote/Serialization/ExceptionSupport.cs

@@ -137,5 +149,8 @@ public Exception ExceptionFromProtoNet(Proto.Msg.ExceptionData proto)

            return obj;
        }
+
+        private string ValueOrNull(string value)
+            => string.IsNullOrEmpty(value) ? null : value;


Default value for strings inside an Exception is null, have to convert proto empty strings to null to make a proper Exception object

Arkatufus · 2021-04-16T20:28:07Z

src/core/Akka.Remote/Serialization/ExceptionSupport.cs

+                if (_defaultProperties.Contains(info.Name)) continue;
+                if (info.Value is Exception exceptionValue)
+                {
+                    var exceptionPayload = ExceptionToProto(exceptionValue);


Old code would fail if it encounters an Exception property inside an Exception, have to make sure that the serializer recurse through these

Arkatufus · 2021-04-16T20:31:32Z

Still need to test that exceptions in Akka.Cluster are serialized properly

src/core/Akka.Tests/Actor/Dispatch/Bug2640Spec.cs

Aaronontheweb · 2021-04-21T20:21:35Z

Going to move this into 1.4.20 just because 1.4.19 is already quite a big release, but this looks good to me thus far

Aaronontheweb

LGTM

Aaronontheweb · 2021-05-05T15:41:05Z

@Arkatufus this test is now failing on every run (racily) on this PR since your changes:

ForkJoinExecutor should terminate all threads upon all attached actors shutting down

Please revert

This reverts commit c34b2a6.

Aaronontheweb · 2021-05-07T01:13:34Z

@Arkatufus seeing this show up in the MNTR for .NET 5 / .NET Core 3.1

Failed to load test details for [Akka.Cluster.Tests.MultiNode.SingletonClusterWithAccrualFailureDetectorMultiNode] test class: System.IO.FileNotFoundException: Could not load file or assembly 'D:\a\1\s\src\core\Akka.MultiNodeTestRunner\bin\Release\net5.0\win10-x64\publish\Akka.Cluster.Tests.MultiNode.dll'. The system cannot find the file specified.
File name: 'D:\a\1\s\src\core\Akka.MultiNodeTestRunner\bin\Release\net5.0\win10-x64\publish\Akka.Cluster.Tests.MultiNode.dll'
   at System.Runtime.Loader.AssemblyLoadContext.LoadFromPath(IntPtr ptrNativeAssemblyLoadContext, String ilPath, String niPath, ObjectHandleOnStack retAssembly)
   at System.Runtime.Loader.AssemblyLoadContext.LoadFromAssemblyPath(String assemblyPath)
   at System.Reflection.Assembly.LoadFrom(String assemblyFile)

That's why everything is running in 15 minutes. Any ideas on this?

Aaronontheweb · 2021-05-07T18:06:01Z

Looks like it's working now - must have been an intermittent issue.

transmax-open-source added 2 commits May 17, 2019 08:50

Enable features that are now in the netstandard2.0 library set (but w…

a43207d

…ere missing from the 1.x library set). akkadotnet#3668

Correct version numbers for Clean target akkadotnet#3668

98680a2

This was referenced May 16, 2019

Fix serialize-messages for Akka.Cluster and Akka.Remote (netstandard2.0) #3791

Closed

Move Build System to .NET Core 2.1, projects to .NET Standard 2.0 and .NET 4.5.2 #3668

Merged

Aaronontheweb reviewed May 16, 2019

View reviewed changes

Aaronontheweb added this to the 1.4.0 milestone May 16, 2019

Aaronontheweb approved these changes May 16, 2019

View reviewed changes

Merge branch 'dev' into remove-1x-netcore-restriction

0e52d7a

heatonmatthew mentioned this pull request May 17, 2019

Fix repeatable failure of a "Tail_chopping_group_router" test case #3785

Closed

Aaronontheweb added 3 commits May 20, 2019 20:42

Merge branch 'dev' into remove-1x-netcore-restriction

5d6c75b

Merge branch 'dev' into remove-1x-netcore-restriction

587a396

Merge branch 'dev' into remove-1x-netcore-restriction

48929a5

Merge branch 'dev' into remove-1x-netcore-restriction

b25daa0

Merge branch 'dev' into remove-1x-netcore-restriction

bf43ec0

Aaronontheweb mentioned this pull request Feb 28, 2020

Remove 1x netcore restriction #4273

Closed

Aaronontheweb modified the milestones: 1.4.0, 1.4.1 and Later Feb 28, 2020

Aaronontheweb modified the milestones: 1.4.2, 1.4.3, 1.4.4 Mar 13, 2020

Aaronontheweb modified the milestones: 1.4.4, 1.4.5 Mar 31, 2020

Update APIApproval list

3dfd0c0

Arkatufus reviewed Apr 16, 2021

View reviewed changes

Merge branch 'dev' into remove-1x-netcore-restriction

6422da1

Arkatufus reviewed Apr 19, 2021

View reviewed changes

src/core/Akka.Tests/Actor/Dispatch/Bug2640Spec.cs Outdated Show resolved Hide resolved

Fix flaky spec

c34b2a6

Arkatufus reviewed Apr 20, 2021

View reviewed changes

src/core/Akka.Tests/Actor/Dispatch/Bug2640Spec.cs Outdated Show resolved Hide resolved

Aaronontheweb modified the milestones: 1.4.19, 1.4.20 Apr 21, 2021

Arkatufus added 2 commits April 23, 2021 00:07

Merge branch 'dev' into remove-1x-netcore-restriction

d0b5302

Merge branch 'dev' into remove-1x-netcore-restriction

5bb8450

Aaronontheweb approved these changes May 3, 2021

View reviewed changes

Merge branch 'dev' into remove-1x-netcore-restriction

e53cc2c

Aaronontheweb enabled auto-merge (squash) May 3, 2021 22:59

Arkatufus added 2 commits May 4, 2021 22:46

Merge branch 'dev' into remove-1x-netcore-restriction

6e33872

Merge branch 'dev' into remove-1x-netcore-restriction

bd7e10c

Arkatufus and others added 3 commits May 5, 2021 22:47

Revert "Fix flaky spec"

68cc15b

This reverts commit c34b2a6.

Fix flaky tests

0956e76

Merge branch 'dev' into remove-1x-netcore-restriction

0a196e0

Aaronontheweb disabled auto-merge May 10, 2021 13:27

Aaronontheweb merged commit 55c1175 into akkadotnet:dev May 10, 2021

This was referenced May 12, 2021

added v1.4.20 release notes #5007

Merged

v1.4.20 Production Release #5008

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove restrictions required by netstandard 1.x but available in 2.0 #3790

Remove restrictions required by netstandard 1.x but available in 2.0 #3790

heatonmatthew commented May 16, 2019 •

edited

Aaronontheweb commented May 16, 2019

Aaronontheweb left a comment

Aaronontheweb May 16, 2019

Aaronontheweb May 16, 2019

Aaronontheweb May 16, 2019

Aaronontheweb May 16, 2019

Aaronontheweb May 16, 2019

heatonmatthew May 16, 2019

Aaronontheweb May 16, 2019

Aaronontheweb May 16, 2019

Aaronontheweb May 16, 2019

Aaronontheweb May 16, 2019

heatonmatthew May 16, 2019

Aaronontheweb May 16, 2019

Aaronontheweb left a comment

heatonmatthew commented May 17, 2019

Aaronontheweb commented Jun 4, 2019

Aaronontheweb commented Jun 26, 2019

heatonmatthew commented Jul 15, 2019

Aaronontheweb commented Jul 16, 2019

Aaronontheweb commented Jul 16, 2019

Arkatufus commented Apr 16, 2021

Arkatufus Apr 16, 2021

Arkatufus Apr 16, 2021

Arkatufus Apr 16, 2021

Arkatufus commented Apr 16, 2021

Aaronontheweb commented Apr 21, 2021

Aaronontheweb left a comment

Aaronontheweb commented May 5, 2021

Aaronontheweb commented May 7, 2021

Aaronontheweb commented May 7, 2021

Remove restrictions required by netstandard 1.x but available in 2.0 #3790

Remove restrictions required by netstandard 1.x but available in 2.0 #3790

Conversation

heatonmatthew commented May 16, 2019 • edited

Aaronontheweb commented May 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

Choose a reason for hiding this comment

Aaronontheweb left a comment

Choose a reason for hiding this comment

heatonmatthew commented May 17, 2019

Aaronontheweb commented Jun 4, 2019

Aaronontheweb commented Jun 26, 2019

heatonmatthew commented Jul 15, 2019

Aaronontheweb commented Jul 16, 2019

Aaronontheweb commented Jul 16, 2019

Arkatufus commented Apr 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arkatufus commented Apr 16, 2021

Aaronontheweb commented Apr 21, 2021

Aaronontheweb left a comment

Choose a reason for hiding this comment

Aaronontheweb commented May 5, 2021

Aaronontheweb commented May 7, 2021

Aaronontheweb commented May 7, 2021

heatonmatthew commented May 16, 2019 •

edited