WeakReference<AttributeObserver>>

[eclipse]

WeakReference>

from milo.

[kevinherron]

It may just be an artifact of a previous design in the server. Older
versions toyed with a reference counting system for UaNodes and the idea
that nodes kept references to other nodes, and it may have just been that I
didn't want AttributeObserver references that were never cleaned up to leak
when orphaned nodes were still hanging onto references (or something like
that).

It doesn't appear at first glance that they are necessary any more.

It's hard to tell because the design and API in the server SDK is still
either in development or in flux, depending where you look.

On Thu, Sep 22, 2016 at 2:52 PM, ergouser [email protected] wrote:

Is there some reason for using a WeakReference for AttributeObservers? I
can think of a number of use-cases where there's a one-to-one relationship
between a node and an AttributeObserver and no reason (other than the
WeakReference) for any other instance to hold a reference.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#54, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAUxMWm31Dnw03gFn5ivOZtNEB0-pAfDks5qsvi0gaJpZM4KEZyw
.

[ergouser]

Vaguely related (or possibly just my ignorance of usage) is that there does not seem to be a way to change the value of a UaVariableNode node at the server without also triggering fireAttributeChanged. If you call "setValue" from the server side (upon reading a value from whatever is the real source of the data) an attributeChanged is generated in the same fashion as if a Client had changed the value - not always a desirable side-effect. The "builder" pattern makes it more difficult to simply subclass the node and add a set method that doesn't trigger the notification.

[kevinherron]

Actually my original intent for the AttributeObserver was for server-initiated attribute changes. That it happens also when a value is set due to a write is coincidence.

In the "real world" systems I've built on this so far, most writes to real data sources, like PLCs, RTUs, etc... are handled in the write call on Namespace, not with observers hanging off each node.

The whole system is actually designed so that you can implement a Namespace without ever even using a UaNode instance. They exist for convenience, but aren't intended to be required.

[ergouser]

Interesting... However, for better or for worse I think that OPC is still mostly about tag/value pairs which lends itself well to the UaNode/Observer model. For example, even though we automatically aggregate PLC register read blocks, they are presented to the user as a single register-tag/value instance/entity.

Put another way, the observer pattern (or a delegate or a subclass) allows the entire read/write operation to be encapsulated in the node structure. Without that, you have to have some additional mapping (from nodes to read/write entities of arrays of values or similar) in the NameSpace.

With the same philosophy, it might be desirable to pass the MethodInvocationHandler into a UaMethodNode.

These are relatively minor tweaks - please don't take them as criticisms. So far everything seems solid and well built.

[kevinherron]

I think removing the WeakReference is a pretty easy sell.

Maybe the AttributeObserver interface can be modified to include a flag or some other contextual information that indicated it was a change instigated by a user/write, or even two separate callbacks on the interface.

It should be possible at some level to know the change is coming from a write operation because write operations either get handled at the Namespace level or they pass through UaNode#writeAttribute which delegates to the AttributeWriter helper. setValue is meant for server/internal use and shouldn't be used directly by implementations to handle writes (unless there is some compelling reason, I guess... but there's a lot of work you'd be re-doing in that case [e.g. all the stuff writeAttribute does]).

I'd like to figure out something that doesn't involve changing any of the setXYZ method signatures for attributes, because that would mean changing the core Node interface.

[ergouser]

I was leaning towards subclassing the nodes, but you have all the variables labeled as "private" - that makes it impossible to implement a subclass that can set the value without also triggering the notification. (Almost all the "set" methods have side-effects, so accessing private variables with set/get is not really an option).

I would strongly suggest that, for a library, absent any pressing reason for a variable to be private, the default protection level should be "protected". This allows subclasses access to the variables while preventing direct access to non-subclasses outside the package.

[kevinherron]

I'm gonna let the idea of making all the attribute variables protected bounce around in my head for the day. It seems harmless enough... I usually lean towards private/final/immutable and composition where possible, so I'm already a little out of my "comfort zone" with these UaNode implementations. It's important to find the right balance between an obvious, safe, and perhaps idealistic implementation and actual usability, so I'm happy to hear your feedback.

This week has been absolutely crazy at work. Over the weekend I should be able to make some of these changes as well as address some of the outstanding issues and in progress PRs I've got going...

[robert-schaft-hon]

Without knowing in detail the code, I recommend that you stay in your comfort zone, kevin.
Making variables protected means, that they belong to the API, must be documented and are nearly impossible to change because you would break someone elses code.

[ergouser]

The "Observer" encapsulation can be made to work. You need to make the "value" field of the variable node accessible and then use reflection to set it.

  valueField = UaVariableNode.class.getDeclaredField("value");
  valueField.setAccessible(true);

You also need to keep a separate list of Observers to maintain a strong reference. Both of which are really kinda nasty hacks.

One more serious issue is that the stack leading to attributeChanged needs to throw. Without that there's no way pass an error back to the client.

Prohibiting subclasses, which is what the "private" variables do, is a major limitation in a library. It's clearly a design choice, but a very restrictive choice. I don't really like the "Builder" pattern in a library, mostly because it requires the complete creation of a Builder for each subclass - but that's again a design choice.

[kevinherron]

One more serious issue is that the stack leading to attributeChanged needs to throw. Without that there's no way pass an error back to the client.

Now we're talking about a serious misuse of what the observers were intended for. I mentioned very early in this exchange they were never meant to be one of the ways to handle writes from the client. It's observation only.

I can't think of any observer-like implementation I've seen that wasn't passive, i.e. had the ability to "veto" changes. That's simply not the intent here.

[ergouser]

One problem with publishing a library is that the users will not follow your "intent" - preferring to use the features you provide in the manner they choose. (Sorry).

But I think this is more of an architectural disagreement (given the current state of the US, I should perhaps state that it's a polite disagreement - I understand and respect your position, but disagree with you). I think that you should be able to encapsulate the behavior of the Node within the node, that is, you should be able to associate an instance with the node that handles reading/writing of that node.

I understand that your idea was that everything should be handled in the NameSpace, but that means that you have two data model, the "node" model and the data model maintained by the NameSpace. I can understand why a second data model might be useful, PLC register blocks are an obvious example, but multiple data models are generally undesirable - it's like violating first normal form in a database.

[kevinherron]

With the interop in Nuremberg being next week and a potential first release for Milo in December, I'm hoping to get to as much of this refactoring and API design done as possible in the next few days and following week.

Today I'm focusing on getting the client squared away and when I switch focus to the server this weekend I'll definitely re-read this thread carefully, ask for input, and hopefully a design emerges that satisfies everyone.

[kevinherron]

Also worth mentioning re: having two data models.

The namespace data model will always be the the primary data model. The node implementations are meant to complement it by providing an easier to use abstraction on top of it, but the namespace will always be king. You'll notice that the entire namespace API surface is designed so that you never actually need to have instances of a Node in memory, which was one of the primary goals of its design (and possibly the reason for some awkwardness).

But this is important because it allows implementations to model a potentially vast amount of nodes without needing to have an in-memory representation for every one of those nodes. Only upon a client subscribing to an attribute of a node does something finally have to reside in memory (the monitored item). Browsing, reading, writing, etc... can all intentionally be implemented without ever using a Node instance.

[ergouser]

Valid point, but I'm not sure what it gets you. You still (or we would still) have to have one model in memory. Your argument is that should be just the NameSpace model, having just the Node model in memory is equally viable.

One thing that might better help you understand is that we already have a fairly large (potentially very large) model that is a graph of interconnected components. Simply attaching a node to the end of (or any point within) the graph is a relatively trivial task. And yes, we could build a structure within the NameSpace, but it would just about exactly parallel the model of the Node structure. We really would have two near-identical models within the server.

Put another way, the entire model within the OPC server is redundant to us (but obviously necessary for OPC functionality), we absolutely only care about having an read/write instance and for that the Node model is potentially the simplest and cleanest.

[kevinherron]

You don't have to model anything in memory inside the Namespace.

One of the motivating scenarios (for me) is modeling some obscenely over-sized ControlLogix tag databases in the address space. Anywhere from hundreds of thousands to million-plus tags. In nearly all cases the number of tags is a result of how users use and abuse of UDTs and basically cause the tag count to explode. But when I'm modelling all my state information for a tag database I'm not modelling the entire thing in memory - I'm only modelling the very base set of symbols and the template definitions. From there I can derive a more usable tag instance for any tag in the controller given only the path.

This same idea is then carried over in the UA namespace - I don't have to have ANY node instances for any of the tags in the controller. I receive a browse, read, write, etc... and then everything I need to know to complete one of those requests is derived at that point and carried out. The only time something needs to reside in memory is when a node gets subscribed to, and even then I don't create a node instance for it, I just have a subscription model that works directly against the DataItem#setValue call.

[ergouser]

Fair enough, and a valid use case, but only one of many. I can clearly see the advantages if the model exists only to support the OPC server.

In our case the model exists regardless, OPC is simply a way to access the model. We also support MQTT, REST, DCOM OPC (please, no), etc. etc as parallel access methodologies, as well as a GUI.

In our case, 80%+ (probably closer to 100%) of the exported OPC tags will be part of a subscription, so lazy creation does not have the benefits that it would in your use case.

[kevinherron]

Well either way I want to find a way to accommodate what you're doing. I've been building this library in a vacuum for much of its history so obviously it was designed around my needs.

I've got the twinkling beginnings of an idea for how to get you what you're looking for, hopefully this weekend I can flesh it out a little and get your feedback.

[ergouser]

Thanks for your help and understanding, and for the library. I suggest that you put any ideas out on branch whenever you have anything - I'll take a look as soon as I can (next week may be a little crazy, but then most weeks are).

[kevinherron]

@ergouser I've done some work on this over the week (and put the implementation to the test at the interop event). I think this is basically what you're asking for... but even if it's not it turns out I really like it so it's probably going to be merged into master once I finish it either way.

I determined that what you really want is an AttributeDelegate, not an AttributeObserver, and these two concepts can peacefully coexist.

An example of using it to implement nodes with dynamic/random values in the example address space is here.

Using the concept to intercept value read/write to implement user based access control can be seen here. The reusable RestrictedAccessDelegate powering this example is here.

The design and implementations aren't quite finalized but I think you can get an idea of what it will look like from this. The end goal is that AttributeDelegates are composable, allowing implementations to be effectively and efficiently combined by composition rather than inheritance (although you could use inheritance if you really wanted).

[kevinherron]

Now updated with a cleaner API and a utility and example for chaining delegates.

[kevinherron]

See #90