JKISoftware / JKI-State-Machine-Objects

Object-oriented framework for LabVIEW based on the JKI State Machine
BSD 3-Clause "New" or "Revised" License
95 stars 54 forks source link

onDependencyMessage 'Error Out' and 'SMO Out' values ignored by framework #70

Closed nathan-murphy closed 2 years ago

nathan-murphy commented 4 years ago

SMO.lvclass:Process.vi ignores the outputs from the onDependencyMessage override VI. I am using a by-value SMO and overriding that function, and this effectively means that the data modified in that override function is lost due to the Process VI not using shift registers.

Additionally, if the onDependencyMessage override VI throws an error, that error is lost. I have added the override function 'Handle Error' to my override of onDependencyMessage, but I think that Handle Error should be added to either the parent function and require calling parent, or added into the SMO Process.vi

francois-normandin commented 4 years ago

Hi @nathan-murphy , You are right that the SMO Out value is not having an impact. Adding shift registers to the SMO Process VI will unfortunately not solve your issue. The real problem is that this output terminal should never have been added in the first place. The onDependencyMessage (and the other callbacks handled only by the SMO base process) should only have a dynamic dispatch input terminal.

SMO is a multiple-process framework, so when you work by-value, the changes in SMO:Process are separate from the changes happening in Child:Process.vi. When the onDependencyMessage callback is used, the SMO:Process receives the message, but there is no by-value synchronization mechanism between the loops.

The way I suggest to use the onDependencyMessage callback in a by-Value application is to use it to send a private message to your Child:Process, just like its public API does to communicate with the process. You should ignore the SMO output... for consistency it should be removed, but to prevent breaking code, it will probably stay as is (with a note in the parent description).

Your second point is also good. The error is not dispatched. It should be handled by the SMO:Process. However, it should not be handled in the same way as the SMO ErrorHandler because this would be an error caused by a dependency, not by itself. A SMO base class has no code to handle a runtime dependency (and should never have such code as it should be unaware of the responsibilities of dependencies used by the child implementation). If it were allowed to merge errors in the parent process, a dependency could deliberately send a message to its caller and force it to error out just to mess with it. Caller behavior would be dependent on its dependencies at run time, which creates a circular contradiction.

nathan-murphy commented 4 years ago

Adding shift registers to the SMO Process VI will unfortunately not solve your issue.

Oddly enough, it did in this case. I am emulating Actor Framework a little with my by-value SMO here, where the Process is analogous to Actor Core and very minimal code is added into the actual Process. The primary function of this SMO is to launch dependencies (UI components) and respond to messages that they send. So in my case, adding a shift register does solve my problem because I am only interested in the by-value data while using the onDependencyMessage override. It's a bit odd, and I hear your point about the hierarchy of processes and the child process won't have access to the same by-value data that the parent does, but in my case - that is okay. Though I don't know if it's okay enough to make this change for everybody using the framework.

I think you have a good point on using the onDependencyMessage to forward the actual contents into the Child:Process. That could certainly work for me.

I also don't necessarily agree with this (and that could just be my own dogma of SMO):

because this would be an error caused by a dependency, not by itself

The way that I've thought about SMOs, error handling, and dependencies is that whoever launches is an SMO is responsible for it, including any errors that it could throw (that aren't caught locally). In all of the applications where I've used SMOs with dependencies, it's been in a controller-launches-views scheme, where the only errors coming from a View would be framework errors or bugs. It's helpful to funnel all errors down to one controller when there are up to 40 UI elements each represented as an SMO.

francois-normandin commented 4 years ago

I like that you use the framework like this (AF-style). This is something that I almost always do, although I don't use the SMO process for this... instead I have a child class which does exactly that. I keep the SMO process free to handle dependencies, start-stop and the likes... and I use the process of my child class as the Actor Core equivalent. All messages go through an "Incoming Request" method that is private to the child process, and all my data is kept by-value in that core. It adds one level in the hierarchy but all the data is ferried serially through this command pattern actor-core-style. You seem to have a similar architecture but using the onDependency message override instead. I definitely see how useful the shift registers are in this case.

About error handling, your perspective is correct if considering that SMO would allow only static dependencies, but because it allows any other SMO class to be a potential run-time dynamic dependency, there is no way to ensure that the SMO would know how to handle all types of SMO errors. It could also be the provider of a shared resource used by other SMOs in the system, which is another context to keep in mind as a potential source of errors in those messages. Although, I do agree that the error needs to be captured by the instance: you should be allowed to easily terminate or restart a class instance if an unhandled error happens. I'm open to reconsidering this position, but I think it we should have a map of the range of possible interaction between dependent and dependee before going this route.

francois-normandin commented 4 years ago

I don't see any negatives to adding shift registers on the SMO process loop. I think it should be added to 1.4. In fact, there are possible problems if the developer uses base SMO protected methods to influence the state of the SMO process... but there are always messy problems with protected-scope methods in frameworks.

I'm not yet convinced about the error handling though. I'd like to keep this discussion alive, it is well worth it.

nathan-murphy commented 4 years ago

but because it allows any other SMO class to be a potential run-time dynamic dependency, there is no way to ensure that the SMO would know how to handle all types of SMO errors

I'm on the same page now. The runtime uncertainty of which dependency may be loaded certainly throws a wrench into this.

I think in my designs that has not been an issue since I only promote framework type errors to the owner and all other errors are handled internally. Basically, if there is an unhandled error in any SMO, then the whole app will shut down. This kind of aggressive error handling works well for my applications because it forces the developer to either prevent error conditions or handle them locally.

In either case, I can see how other application designs wouldn't appreciate being shut down on any error.

One thing that may help is having a method of differentiating between static vs. dynamic dependencies. Then if the error is with a static dependency the SMO Error Handler is called, but for a dynamic dependency there could be another error handler override.

francois-normandin commented 3 years ago

Hi @nathan-murphy ,

I revisited the SMO 1.4 branch this week and came to the conclusion that there is no problem with handling errors in Handle Error override when those errors come from the dependencies. I've revisited the Self-Terminate mechanism to be more robust (and not skip on waiting for child process synchronization...).

Since the errors received from dependencies are not propagated through the shift register, the addition of the "Handle Error" callback at the end of the iteration will allow for the developer to override and provide self-termination properties to the SMO by flipping the auto-stop flag in the override.

image

As such, I will be promoting a few changes to the 1.4 release that address this case:

  1. Incoming unhandled errors from dependencies will be merged and fed in the "onUnhaadledDependencyError" callback where the developer can either handle the error or thrall it to the output where the framework will handle it by calling the Handle Error method. image

  2. Dependency overrides will now handle the errors instead of being ignored as a no-op. This will handle the use case of capturing any framework-related errors, but also allow for capturing unhandled callback errors. Re-reading this thread multiple times in the past few days, and inspecting the code, I came to the conclusion that my fears of having a dependency "inject" errors in the owner are unfounded: the overrides are part of the owner class and therefore are not subject to unspecified errors added at runtime. image

  3. The error output terminal will retain its "non-shift-registered" status in the loop. This is done to maintain current implementation behaviors so that the changes do not affect any existing code. It is my belief that the proposed changes do not modify the behavior of the SMO from 1.3.0 (so calling it 1.4 is actually legitimate).

  4. Self-terminate behavior has been ruggedized and added as a parallel task to the SMO process. The activity diagram will be simplified by this change as it will centralize the self-termination in the base process wrapper method instead of being distributed in any child process override which called the Self-Terminate method. Furthermore, it fixes a problem with self-termination that prevented sync of the child processes during shutdown. The new behavior will use a dedicated private event unique to the process thread, which will self-contain the changes and provide a more stable experience.

image

edit: Currently in feature branch "feature/stable140" (as of 2021/05/02)

nathan-murphy commented 3 years ago

The changes look great! I want to check my understanding on my initial question/issue. If there is an error that is generated inside a child's override of onDependencyMessage, even though that error may have been generated by an unknown child class, that error should be handled by the existing "Handle Error" (which should also be overridden by the child class).

Since Handle Error is also used in the child process, this implementation assumes that errors in the SMO process are handled the same way as ones that can be generated in the onDependencyMessage. Is this your intention?

To be honest, I rarely override Handle Error so this proposal works well for me. In my SMO-based applications,, any error that makes it to Handle Error are logged and then the whole application is shut down.

francois-normandin commented 3 years ago

Hi @nathan-murphy ,

yes, you are correct. I realized a flaw in my initial reaction, which was that there could be unknown errors injected in the caller... but that fear is not founded, because the caller is still responsible for launching both static and (self-launched) dynamic dependencies. Therefore, it should be aware of their APIs, error codes and how to handle them. If it encounters an unhandled error, it should safely terminate itself. Reporting the reason for termination is, although optional, obviously a must.

The only thing I'm going to investigate further is the possibility by a 3rd-party (another SMO or top-level BD) to inject a dynamic dependency into a dependee by using the public method. I believe this is the use case I was worried about and it is definitely an issue since the dynamic dependency launcher is a public method (for convenience). There will need to be a distinction, at the framework level, between error handling from a dependency that is injected by the SMO itself, and a dependency that is called on a flat block diagram using the public method. Such a dynamic dependency is not a dependency that the SMO was meant to understand in the first place, so it is ok to stop on unhandled error too.

francois-normandin commented 3 years ago

For convenience, the "Launch Dynamic Dependency" is public and allows the developer to quickly add a dependency at runtime... This is meant to be helpful in laying out a flattened dependency injection instead of wrapping everything in class-member methods.

What I mean by this is the following. In the screenshot below, the "System" SMO gets a "Pump" SMO added as a dynamic dependency. image

What this allows is to build a system from a top-level diagram and have all the subsystems being accessible without diving into subVIs... It also makes the System SMO unaware the types of modules that it could be responsible of. That's where I have a concern.

This is a perfectly reasonable way to code. A very convenient and expeditive way to whip up a quick app for testing. Personally, I only use this schema for testing, demos or debugging... and I always call the dynamic launcher wrapped into a Pump::LaunchSpecificDynDependency method to make sure I know what is going on.

I'll introduce a mechanism to keep this way of injecting code at the top-level, but will make it a special use case when this is done after a dependency is owned by another module... Error handling should be different in both circumstances.

nathan-murphy commented 3 years ago

It also makes the System SMO unaware the types of modules that it could be responsible of.

I'll give you the benefit of the doubt here since I know that you're not advocating for monolithic applications with no plugins or dynamic dependencies. But let me go down this rabbit hole... The System SMO knows that any dependency is an SMO, what else should it need to know?

I think that question is only answerable at application specific level. Sometimes it may be enough to know that it is just an SMO and that it can send errors, send messages, etc. Other times, I can see why you'd want to have some knowledge (at least of the abstract classes or interfaces) of the SMOs being launched as dependencies.

Because of this, I'm not sure that error handling or the way the framework behaves in either case should change.

Maybe I've just been abusing the public API for launching dependencies - is there a better method?

francois-normandin commented 3 years ago

You have not been abusing it. I think it's different coding style. My aim, when advancing features in SMO framework, is to never cater to only one type of coding style. The framework should be complete enough to allow for all styles. Case in point: the SMO framework allows byValue, byRef and a mixture of both within the same module. It also does not enforce a messaging layer, so that it can be extended to choose what the developer needs. It does not enforce that you use the Static Dependency enumeration to launch dependencies, you can use the dynamic launcher as well... All these choices are made on purpose, so that no one is forced into a particular way of thinking... In my opinion, it would seriously limit the creativity of the developer. :-)