[osmosis-dev] read-only entities

[Brett Henderson]

I'll have a think about it.  I don't have any huge issues with the 
approach.  I'd also like to think about is Marcus's suggestion of using 
interfaces instead but I haven't thought it through yet.

One minor thing will be that it will take some vigilance to make sure 
this pattern isn't broken.  In other words, if new set methods are added 
to sub-classes the check will have to be added and could be easy to 
forget.  This doesn't happen too often though so possibly not an issue.

We'll also have to figure out how we lock down the collections.  We 
currently return collections of tags/wayNodes, etc to clients.  We can 
potentially wrap these using Collections.unmodifiableCollection if the 
class is read only which might solve the problem.

Jochen Topf wrote:
> I thought about the read-only entitys some more and still dislike any solution
> where we have extra classes for them. The only way I could imagine doing it
> is with some kind of wrapper class for all entities that implements a
> copy-on-write. But I can't think of a way to implement this without copying
> too much code.
>
> So here is my proposed solution:
>
> The Entity class gets an attribute "boolean read_only". On creation it is
> false, it can be set by calling entity.makeReadOnly(). All setter function
> check this attribute and throw an exception if ready-only is set.  There is
> also a method isReadOnly() to check this attribute and a cloneIfReadOnly()
> which will return a clone of the entity (with the read-only attribute not set)
> if it is read only and the entity itself if not.
>
> The --tee task and all other tasks that somehow create threads have to either
> create a deep copy of an Entity for each thread or use the same entity and call
> .makeReadOnly() on it (preferred). That takes care of thread-safety.
>
> Any task that knows that it will not change any entities it gets, simply calls 
> getEntity() as before in the process() method of a task. It doesn't care whether
> the read-only attribute is set or not.
>
> If the tasks knows beforehand that it will probably change the entity, it calls
> getEntity().cloneIfReadOnly() instead of just getEntity().
>
> If the task only sometimes changes an entity it can still call getEntity() and
> do the cloneIfReadOnly() at any convenient point when it has found out that
> it will need to change this entity.
>
> So, this hole process means that there is no copying penalty for the common
> case. A copy will only be done if there is a --tee *and* some task after it
> changing an entity. The penalty of creating a deep clone of an entity is only
> ever incurred when it is really needed. Of course there is the added overhead
> on checking one boolean every time a setter method is called. This is not
> strictly necessary, because well-behaved tasks would never call a setter method
> on a read-only entity, but of course it must be done in case there is a
> misbehaving task.
>
> Not much change in the current code is needed to implement this and it is easy
> for writers of a task, they just have to know about the cloneIfReadOnly() and
> call it if needed.
>
> The only drawback I can see is that some compile time checks aren't possible
> any more. The setter methods have to check at run time. But I think in this case
> thats just a very minor drawback.
>
> We can also throw in another goodie: One common way an entity might change is
> if the tags change but nothing else. Or the nodes in a way change and nothing
> else. So in addition to the cloneIfReadOnly() we could have methods that will
> clone an entity but leave the tags or nodes lists empty or so. So we don't have
> to have the overhead of deep cloning the entity and throwing half of it away
> right after that. But we can figure out how to best do this later.
>
> Jochen
>