louthy / echo-process

Actor library for C# with additional modules that support persistence to Redis, as well as JS integration
MIT License
116 stars 18 forks source link

Questionable disposable by Actor #28

Closed TysonMN closed 6 years ago

TysonMN commented 6 years ago

For clarity, @StefanBertels was motivated by this line of code in the Actor class when he opened language-ext issue #205.

TysonMN commented 6 years ago

@StefanBertels, I would like to discuss the lifetime managment of the application you have written with echo-process.

On this line, the previous state might be overwritten with new state. After this line, the Actor<,> class is no longer able to call Dispose on state were it to implement IDisposable (like it does through its own Dispose method here).

The user of Actor<,> could call Dispose on this old state inside the code that defines the field actorFn, which came from the constructor argument actor in the only constructor of `Actor<,>. The scope of this constructor is internal but is exposed in two places that accept arbitrary delegates (with the correct type) for this parameter.

So @StefanBertels, how are you managing the lifetime of all state instances before the "last one"? Specifically, which code is calling Dispose on those instances?

louthy commented 6 years ago

If the user manually calls Dispose on the state then it's their own stupid problem. The point of the state is it belongs to the Process.

louthy commented 6 years ago

Just to clarify. Anybody using a disposable state would usually have a Process that manages a single disposable reference for the lifetime of the Process. So, for example, a Process that manages a database connection.

If you want to manage a sequence of changing disposable references then you need to (in the inbox function) dispose the current state and return the new state which will be cleaned up automatically when the Process shuts down.

There is no magic with IDisposable - but the Process system essentially provides the option of a shutdown operation via IDisposable. That's its key purpose.

TysonMN commented 6 years ago

Suppose that I I wanted to use echo-process with two Actor<,> instances. Furthermore, suppose each of these instances also had their own state instances but that both state instances shared a reference to a disposable object that was provided to each in their constructors.

I don't see how the current lifetime management approach by Actor<> supports this use case. One Actor<> instance could be disposed very "early", long before the second Actor<> instance is down with its work. But this will lead to an ObjectDisposedException being thrown because Actor<> called Dispose on its state instance without knowledge of the proper lifetime of that object.

StefanBertels commented 6 years ago

Why should two actor share some disposable state?

The actor concept (as I understand) means actor is owner of its state. Shared state might be bad design. At least you could handle this in sone custom way.

Anyway echo has this contract for setup/shutdown. We really need some way to do some clean shutdown (or restart) and I don't see why we shouldn't use IDisposable here. Seems like a good fit here.

louthy commented 6 years ago

Sharing state is the exact opposite purpose of the actor model. If you share your state then you’re fair game for bugs and it’s not this library’s problem. On Sun, 31 Dec 2017 at 00:28, Stefan Bertels notifications@github.com wrote:

Why should two actor share some disposable state?

The actor concept (as I understand) means actor is owner of its state. Shared state might be bad design. At least you could handle this in sone custom way.

Anyway echo has this contract for setup/shutdown. We really need some way to do some clean shutdown (or restart) and I don't see why we shouldn't use IDisposable here. Seems like a good fit here.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/louthy/echo-process/issues/28#issuecomment-354577231, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5kk1jGohLTQ-I0byDUwhz1-1sGdqb0ks5tFtUigaJpZM4RPr1e .

TysonMN commented 6 years ago

I see. I am less familiar with the actor model. I retract my argument about shared state.

TysonMN commented 6 years ago

@StefanBertels, in your Actor<Option<D>, T> state, what is your type D that is disposable?

StefanBertels commented 6 years ago

One of my use cases is:

T is IDisposable

It is the return value of Subscribe(...) called on a hot observable.

TysonMN commented 6 years ago

For that use case, does your code need to remember if your state opt is in the Some or None state or would it suffice to change your state to new System.Reactive.Disposables.CompositeDisposable(opt)?

StefanBertels commented 6 years ago

I suggest to close this issue because I think it is clear that actor state might be disposable and actor management is responsible for disposing state (together with creating new state using setupFn).

Regarding disposable Option as actor state type let's continue in https://github.com/louthy/language-ext/issues/205

TysonMN commented 6 years ago

I don't mind if this is closed but I don't think it is clear that "actor management is responsible for disposing state".

louthy commented 6 years ago

Really not important.