(FYI I am unable to post actual code in question)

I have an interface

interface IEntity {
    List<IEntity > getChildren(); //return list of children, safe to iterate i.e. a copy
    List<IEntity > addChild(IEntity child); //add a child
    List<IEntity > getParents(); //return list of parents, safe to iterate i.e. a copy
    List<IEntity > addParent(IEntity parent); //add a parent
}

The requirement is that the parent/child lists stay in sync. Thus each time addChild is called, the internals of the method need to ensure that the parent collection of the child is updated as well. The potential infinite loop (addChild -> addParent -> addChild) can be resolved by updating the internal collection before attempting to update the correspond parent/child.

The non-thread safe code is relatively straightforward. However once thread-safety is considered, it gets more complex. I believe deadlock situation is created if each method simply locks it's own collection. e.g.

addChild(IEntity child) {
    synchronized(children) {
        if(!children.contains(child)) {
            ...
            child.addParent(this);
            ....
        }
    }
}
...
addParent(IEntity parent) {
    synchronized(parents) {
        if(!parents.contains(child)) {
             ...
             parent.addChild(this)
             ...
        }
    }

With each instance locking its internal collection, the aforementioned loop (addChild -> addParent -> addChild) results in a deadlock because the lock needs to be acquired before the contains test.

It seems the internals of both methods addChild and addParent need to lock the corresponding collection of the other entity, so that we can guarantee that when the method exits the collections are in sync. However, neither instance has direct access to the internal collections. All parameters of the IEntity interface type.

I considered locking on the instance itself e.g. ...

addChild(IEntity child) {
    synchronized(this) { //Always lock parent first to prevent deadlocks
        synchronized(child) { //Generates a warning on locking method parameters
        ....
        }
    }
}

addParent(IEntity parent) {
    synchronized (parent) { //Always lock parent first to prevent deadlocks
        synchronized (this) { //Generates a warning on locking method parameters
        ...
        }
    }
}

While I think this can work, it seems that locking on method parameters is dangerous. From what I can tell, the problem in that thread is different than this. So is it possible that this is a valid usage?

Some of this stems from the fact that the implementation is somewhat trying to implement a feature that is not fully supported by the interface. i.e. the interface offers no way to lock internal fields, but the implementation kinds of needs to. Re-designing is possible to a certain degree.

Some other points:

• these are not database entities, so I cannot delegate that synchronization to any other component
• There will indeed be other implementations of these entities, hence needing to use the interface.

Edit: Until a better option presents itself, I am using a global lock for the entire hierarchy, which must be obtained before adding/removing any entities.

(FYI I am unable to post actual code in question)

I have an interface

interface IEntity {
    List<IEntity > getChildren(); //return list of children, safe to iterate i.e. a copy
    List<IEntity > addChild(IEntity child); //add a child
    List<IEntity > getParents(); //return list of parents, safe to iterate i.e. a copy
    List<IEntity > addParent(IEntity parent); //add a parent
}

The requirement is that the parent/child lists stay in sync. Thus each time addChild is called, the internals of the method need to ensure that the parent collection of the child is updated as well. The potential infinite loop (addChild -> addParent -> addChild) can be resolved by updating the internal collection before attempting to update the correspond parent/child.

The non-thread safe code is relatively straightforward. However once thread-safety is considered, it gets more complex. I believe deadlock situation is created if each method simply locks it's own collection. e.g.

addChild(IEntity child) {
    synchronized(children) {
        if(!children.contains(child)) {
            ...
            child.addParent(this);
            ....
        }
    }
}
...
addParent(IEntity parent) {
    synchronized(parents) {
        if(!parents.contains(child)) {
             ...
             parent.addChild(this)
             ...
        }
    }

With each instance locking its internal collection, the aforementioned loop (addChild -> addParent -> addChild) results in a deadlock because the lock needs to be acquired before the contains test.

It seems the internals of both methods addChild and addParent need to lock the corresponding collection of the other entity, so that we can guarantee that when the method exits the collections are in sync. However, neither instance has direct access to the internal collections. All parameters of the IEntity interface type.

I considered locking on the instance itself e.g. ...

addChild(IEntity child) {
    synchronized(this) { //Always lock parent first to prevent deadlocks
        synchronized(child) { //Generates a warning on locking method parameters
        ....
        }
    }
}

addParent(IEntity parent) {
    synchronized (parent) { //Always lock parent first to prevent deadlocks
        synchronized (this) { //Generates a warning on locking method parameters
        ...
        }
    }
}

While I think this can work, it seems that locking on method parameters is dangerous. From what I can tell, the problem in that thread is different than this. So is it possible that this is a valid usage?

Some of this stems from the fact that the implementation is somewhat trying to implement a feature that is not fully supported by the interface. i.e. the interface offers no way to lock internal fields, but the implementation kinds of needs to. Re-designing is possible to a certain degree.

Some other points:

• these are not database entities, so I cannot delegate that synchronization to any other component
• There will indeed be other implementations of these entities, hence needing to use the interface.

Edit: Until a better option presents itself, I am using a global lock for the entire hierarchy, which must be obtained before adding/removing any entities.

• java
• synchronization
• thread-safety

• is there one big body of entities, or do multiple threads need to be creating hierarchies of entities in parallel? – jtahlborn Commented Nov 19, 2024 at 21:30
• Multiple threads may be altering the hierarchy at one time. – user25308907 Commented Nov 21, 2024 at 13:00

1 Answer 1

I think maybe this could work... if not, knock slowly! :)

addChild( IEntity child ) {
    addFamily( child, false );
}

addParent( IEntity parent ) {
    addFamily( parent, true );
}

synchronized addFamily( IEntity xx, boolean parent ) {
   if( parent ) {
      if( ! parents.contains( xx )) {
             ...
         parent.addChild( this )
             ...
      }
   }
   else {
      if( ! children.contains( xx )) {
            ...
         child.addParent( this );
            ....
      }
   }
}