Fix TestKitBase and Option<T> #4603

Arkatufus · 2020-11-03T18:04:47Z

Closes #4496.

Original problem was that

Option<T> failed to report that it has no value when passed a null value.
TestKitBase did not check for Option<Config>.None

IgorFedchenko · 2020-11-03T18:15:08Z

src/core/Akka/Util/Option.cs

@@ -30,7 +30,7 @@ public struct Option<T>
        public Option(T value)
        {
            Value = value;
-            HasValue = true;
+            HasValue = value != null;


I was always thinking about that - but what if null in a valid value for this optional thing? What if null means "there is a value, it is obtained from external source/whatever, it is initialized, and it is null right now"?
So I did not ever change that. Still not sure if we need to do that...

No, by definition Option will return None or Some, what is the point of using Option if you still allow null to leak through? you wouldn't fix the original NRE problem.

Even with this, you'll still have NRE problem if you forgot to check for None.

In essence, our Option<T> implementation is a half assed attempt to fix NRE problem, its just moving the problem somewhere else.

If you still need a null, then what you need is a tri-state object, where its state can be null, none, and some

Yeah, it even broke our own code.
This is stupid, I feel like I'm chasing my own tail.

This is a good fix actually, I would love to see exception thrown here too - but maybe this should be done on some breaking change release, i.e. Akka.NET 1.5, or even 2.0, because chances are that many users will need to debug their app and fix things.

I think the proper thing to do right now is to implement the right IOption<T> interface but make it internal and keep the old Option<T> struct for public consumption.
But this will require so so much work to convert every Option<T> in our code x__x

If we want to go full rage mode, switch to C# 8 and make the input type non-nullable

Why do we need the IOption<T> interface exactly? I get the Some<T> and None<T>

…nput

Aaronontheweb

Left some comments

Aaronontheweb · 2020-11-03T21:58:21Z

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

@@ -1292,7 +1292,9 @@ namespace Akka.Streams.Dsl
        public static Akka.Streams.Dsl.Flow<TIn, TOut2, TMat> Batch<TIn, TOut, TOut2, TMat>(this Akka.Streams.Dsl.Flow<TIn, TOut, TMat> flow, long max, System.Func<TOut, TOut2> seed, System.Func<TOut2, TOut, TOut2> aggregate) { }
        public static Akka.Streams.Dsl.Flow<TIn, TOut2, TMat> BatchWeighted<TIn, TOut, TOut2, TMat>(this Akka.Streams.Dsl.Flow<TIn, TOut, TMat> flow, long max, System.Func<TOut, long> costFunction, System.Func<TOut, TOut2> seed, System.Func<TOut2, TOut, TOut2> aggregate) { }
        public static Akka.Streams.Dsl.Flow<TIn, TOut, TMat> Buffer<TIn, TOut, TMat>(this Akka.Streams.Dsl.Flow<TIn, TOut, TMat> flow, int size, Akka.Streams.OverflowStrategy strategy) { }
+        [System.ObsoleteAttribute("Deprecated. Please use Collect(isDefined, collector) instead")]


Aaronontheweb · 2020-11-03T21:59:46Z

src/core/Akka/Util/Option.cs

@@ -30,7 +30,7 @@ public struct Option<T>
        public Option(T value)
        {
            Value = value;
-            HasValue = true;
+            HasValue = value != null;


If we want to go full rage mode, switch to C# 8 and make the input type non-nullable

Aaronontheweb · 2020-11-03T22:00:44Z

src/core/Akka/Util/Option.cs

@@ -30,7 +30,7 @@ public struct Option<T>
        public Option(T value)
        {
            Value = value;
-            HasValue = true;
+            HasValue = value != null;


Why do we need the IOption<T> interface exactly? I get the Some<T> and None<T>

Arkatufus · 2020-11-03T22:05:38Z

IOption<T> is implemented to simulate the scala implementation to the T, it would make Option unambiguous, a None is always a None and a Some will always contain something, it makes it save to do a typed switch on the type of the returned value.

We don't need it per se, but by implementing it we can separate the internal implementation of Option with the one that is being used by user, just in case changing Option will break someone elses code. Pretty much a bandaid with backward compatibility.

Arkatufus · 2020-11-03T22:07:30Z

But it will be a very big bandaid, akin to using a nuclear warhead to kill an ant.

Arkatufus · 2020-11-03T22:10:57Z

The big fear right now is that, even changing that line in Option might break someone elses code.
It already break ours, we're also (ab)using Option as a tri-state return value inside the Collect stream dsl.

Aaronontheweb · 2020-11-03T22:11:20Z

As long as:

It doesn't take long to implement;
Doesn't introduce additional complexities / issues in terms of API compatibility; and
Doesn't introduce lots of new allocations.

Then I think we should do it "the right way"

Arkatufus · 2020-11-03T22:14:09Z

Yes, it will take awhile to implement.
We can change it as carefully as possible so that it doesn't change the public API
It will probably have some memory impact.

Arkatufus · 2020-11-03T22:18:37Z

Memory impact will most probably came from stack allocation, I think struct was chosen exactly because of this.

Aaronontheweb · 2020-11-03T22:36:15Z

Memory impact will most probably came from stack allocation, I think struct was chosen exactly because of this.

that's fine then

Aaronontheweb · 2020-11-03T22:36:34Z

as long as we don't allocate on every Option.[name of read method]

Arkatufus added 2 commits November 4, 2020 01:02

Option<T> erroneously reports HasValue when Value is null

9d9cccb

TestKitBase failed to check if the returned Option<Config> is None

1d25c14

Arkatufus mentioned this pull request Nov 3, 2020

Multiple tests run fails with Akka.TestKit 1.4.8 (worked fine with 1.4.7) #4496

Closed

IgorFedchenko reviewed Nov 3, 2020

View reviewed changes

Arkatufus added 2 commits November 4, 2020 01:21

Merge branch 'dev' into #4496_TestKitBase_bug

ddeb55b

Fix Stream.Dsl.Collect to not depend on null return value to filter i…

1b3e66e

…nput

Aaronontheweb reviewed Nov 3, 2020

View reviewed changes

Arkatufus mentioned this pull request Nov 4, 2020

Need to harden Option<T> to avoid gotcha bugs #4604

Closed

Aaronontheweb merged commit ad8c0d1 into akkadotnet:dev Nov 4, 2020

Arkatufus deleted the #4496_TestKitBase_bug branch March 8, 2021 13:29

Arkatufus restored the #4496_TestKitBase_bug branch April 22, 2022 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TestKitBase and Option<T> #4603

Fix TestKitBase and Option<T> #4603

Arkatufus commented Nov 3, 2020 •

edited

Loading

IgorFedchenko Nov 3, 2020 •

edited

Loading

Arkatufus Nov 3, 2020

Arkatufus Nov 3, 2020

Arkatufus Nov 3, 2020 •

edited

Loading

Arkatufus Nov 3, 2020

Arkatufus Nov 3, 2020

IgorFedchenko Nov 3, 2020

Arkatufus Nov 3, 2020 •

edited

Loading

Aaronontheweb Nov 3, 2020

Aaronontheweb Nov 3, 2020

Aaronontheweb left a comment

Aaronontheweb Nov 3, 2020

Aaronontheweb Nov 3, 2020

Aaronontheweb Nov 3, 2020

Arkatufus commented Nov 3, 2020 •

edited

Loading

Arkatufus commented Nov 3, 2020

Arkatufus commented Nov 3, 2020

Aaronontheweb commented Nov 3, 2020

Arkatufus commented Nov 3, 2020

Arkatufus commented Nov 3, 2020

Aaronontheweb commented Nov 3, 2020

Aaronontheweb commented Nov 3, 2020

Fix TestKitBase and Option<T> #4603

Fix TestKitBase and Option<T> #4603

Conversation

Arkatufus commented Nov 3, 2020 • edited Loading

IgorFedchenko Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arkatufus Nov 3, 2020 • edited Loading

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

Arkatufus Nov 3, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arkatufus commented Nov 3, 2020 • edited Loading

Arkatufus commented Nov 3, 2020

Arkatufus commented Nov 3, 2020

Aaronontheweb commented Nov 3, 2020

Arkatufus commented Nov 3, 2020

Arkatufus commented Nov 3, 2020

Aaronontheweb commented Nov 3, 2020

Aaronontheweb commented Nov 3, 2020

Arkatufus commented Nov 3, 2020 •

edited

Loading

IgorFedchenko Nov 3, 2020 •

edited

Loading

Arkatufus Nov 3, 2020 •

edited

Loading

Arkatufus Nov 3, 2020 •

edited

Loading

Arkatufus commented Nov 3, 2020 •

edited

Loading