Bug in ActorSystemState? about language-ext HOT 9 CLOSED

Yup, that's wrong. Not quite sure how the else part ended up identical, but a lot of ActorSystemState is a legacy from the early system. Most of that code sits in ActorContext.GetDispatcher() now. GetItem is purely used by the root actor to for generating the 'system' actors (like user, dead-letters, etc.). So the else would never be called.

However just to make sure I have made the else throw an exception.

from language-ext.

Ah so "pid.Head() == RootProcess.Id" is asking if the process is local / on this machine??

from language-ext.

@Jagged Yep, the root process has a name which is either root if it's not part of a cluster or whatever the second parameter is when calling Cluster.connect(...). So if the name doesn't match the root then the message needs to be dispatched. It's slightly more complex now that there's dispatch to local, JS (via SignalR) and Redis - that complexity is now dealt with in ActorContext.GetDispatcher().

ActorSystemState was originally the controller of the whole actor-system as-well as being the root actor. But the benefits of all messages going through one 'master' process turned into pain-points of messaging deadlocks, so I broke it all out into the static ActorContext. The root process is now gutted in terms of functionality; it does however need to bootstrap the user and system processes, so it still needed some of the factored out functionality duplicated internally. That's what GetItem is.

I suspect the false clause would never be called, but having the exception throw in there will make absolutely sure.

from language-ext.

Glad I mentioned it then! I felt silly not being able to fix it myself, but the code in this area is rather opaque, and screaming out for documentation! Or better still, make the code self-documenting thru use of explaining variables, perhaps renaming some routines, and breaking some of the larger ones down into manageable bits.

I'm more than happy to roll up my sleeves and helping with that, but I guess I'm wary of stepping on your toes! You might find some routines unrecognisable when I'm done! =P (but hopefully clearer!!)

I've noticed that quite a bit of code still deals with "raw" reference types that can be null, having to continually check all over the place. I'd like to see how we might be able to make use of Some{T} etc.

Ultimately, I'm keen to help out, and see it an opportunity to get familiar with the functional side of things in depth.

-Will

from language-ext.

but the code in this area is rather opaque, and screaming out for documentation! Or better still, make the code self-documenting thru use of explaining variables, perhaps renaming some routines, and breaking some of the larger ones down into manageable bits.

Not 100% sure I agree on its opaqueness; obviously I wrote it, so I have a different view on the code. It's not a trivial system, so there's likely to be 'first look opaqueness', especially when it comes to a paradigm which is foreign to C# (the actor model).

I'm also not sure what you're referring to in terms of poorly named functions or large routines? The way GetItem function which I agree is poorly named made a lot more sense when the ActorSystemState did have its own Key-Value state --- so there may be some legacy names which I agree need fixing.

I generally prefer smaller functions and conciseness rather than the enterprise naming style of ActorFactoryServiceManager (trite example, but hopefully you get my point). Sometimes my drive for expression-based code can be terse, but I personally figure the benefits of robust code outweigh the initial difficulty when returning to code.

I'm more than happy to roll up my sleeves and helping with that, but I guess I'm wary of stepping on your toes! You might find some routines unrecognisable when I'm done! =P (but hopefully clearer!!)

I'm happy for you to do this, but I would advise that any changes you're planning to consult me first (rather than just submitting a pull-request). The main reason is to reduce any wasted effort on your part. My slight concern based on your comments is that it would just be a 'style war'. That sounds more aggressive than I mean it to be. But if as you say you're just picking up the functional baton, my concern is that the issues you see are actually just the difference between OO and functional.

I'm primarily interested in pull-requests that add value. Making the code clearer definitely adds value, but not a huge amount. The areas that you could add tons of real value (I feel) are:

• Documentation (especially on the public interface) adds a ton of value for the end user.
• More unit tests, especially for the core types and functions that doesn't have them.
• Implementation of missing features (like crash strategies for actors).
• Audit of the code for potential bugs

There has never really been a road-map for the project, because initially I built it for myself and have been adding to it as I needed new features. If you have suggestions of new features then I'm open to that also.

I've noticed that quite a bit of code still deals with "raw" reference types that can be null, having to continually check all over the place. I'd like to see how we might be able to make use of Some{T} etc.

Some<T> is less useful that it should be unfortunately. For example this code won't compile, even though List<string> is an IEnumerable<string>

        public static void Foo(Some<IEnumerable<string>> val)
        {
        }

        public void SomeCastTest1()
        {
            var some = new Some<List<string>>(new List<string>() { "a", "b", "c" });
            Foo(some);
        }

So Some<T> changes can be a breaking change and can also be frustrating to use. It's annoying for sure. It can work very well for concrete types, but this library rarely uses them. So we're back in the null check era.

Please don't let my comments put you off. I really value anyone getting involved and helping to improve the library. But if you can just bear my concerns in mind that will make the whole process much smoother :)

from language-ext.

My slight concern based on your comments is that it would just be a 'style war'
But if you can just bear my concerns in mind that will make the whole process much smoother

Thanks for taking my comments so graciously, I have to remind myself that you don't know me from a bar of soap. Sometimes my enthusiasm can get away with me, and other times I can come across quite blunt (oops, sorry!). It's simply because I give a damn, and hoping I'm more than a help than a hinderance! =)

Please keep the feedback coming, it's appreciated!

Some is less useful that it should be unfortunately.

I had a quick look at it last night and was, um, surprised - I didn't know it could hold nulls and be treated as None.

Update: It's been hardened, see PR. I hope that means we can now use Some wherever we're expecting a non-null argument, and allow us to remove the null checks everywhere.

Hardening code and checking for logic faults is somewhat of a specialty of mine, so if you're happy for me to proceed I'll start tidying up the null checks. It will be a breaking change as the function parameters and/or return values will changed from naked Ts to SomeTs.

Thoughts?

from language-ext.

Update: It's been hardened, see PR. I hope that means we can now use Some wherever we're expecting a non-null argument, and allow us to remove the null checks everywhere.

I definitely don't want that. I think you may have missed my previous comment about the covariance issues - that is a major problem for a library that will work with generic types a lot. Also for a library that could be used in performant situations it's unwise to do unnecessary allocations.

I was actually minded to remove Some from the library altogether, because C# doesn't have a concept of a default constructor for struct, so although they're useful as argument checks where a T is implicitly converted to a Some<T>, I can't guarantee that a caller didn't instance the Some via an array or member variable. e.g.

    var x = new Some<object>[10]();    // x == 10 uninitialised Some<object>

    class Foo
    {
        Some<object> Bar;    // This is uninitialised also
    }

Breaking changes are definitely a no-no as well. The library is heavily used now, so we have to have very good reasons for breaking changes. I don't think this is a good one.

(Take a look at a previous issue discussion for the full low-down on the problems with Some: #22)

from language-ext.

Re uninitialised
Yes I'm aware that Some could be uninitialised in that edge case (shame we can't stop it! ...yet C#7 maybe ...). I was trying to take a pragmatic solution to it so Some could be used to protect against nulls in most cases, detecting as soon as possible that nulls/uninitialised values existed (hence the checks put into all properties). But if you're going to drop it then it doesn't matter.

Re it's unwise to do unnecessary allocations
Little confused there, structs don't do allocations.

Re breaking changes
I suspected that might be the case, hence why I queried it. But there was a chance that the library wasn't set in stone yet, and changes could be made to harden it. Worth asking anyway =)

from language-ext.

@Jagged

Little confused there, structs don't do allocations.

Yep, they do. structs can be either stack or heap based and also have a copying overhead. It's definitely a concern for some aspects of this library (like the collections for example).

Worth asking anyway =)

Indeed :) Any public interface would need a very good reason to be changed, The last time I did that was implementing the Map, Lst, Stck, Que and Set types to remove the dependency on System.Collections.Immutable and before that it was to make the fold functions consistent across the library. Where possible I try to mitigate the effects of a change. For example Lst still derives from the same types that System.Collections.Immutable.ImmutableList did. Or I used the Obsolete attribute to hold on to previous function declarations. The fold changes meant a re-ordering of the parameters in the fold delegate - there's no way that couldn't be a breaking change - but it was the right thing to do.

from language-ext.