1

Currently using the following...

remoteDataObject = <Remote call to get data>

synchronized(localDataObject)
{
    localDataObject = remoteDataObject;
}

Coverity reports - Bad choice of lock object.

Bad choice of lock object (BAD_LOCK_OBJECT)7. lock_on_assigned_field: Locking on the object referenced by field localDataObject. This lock acquisition may race with another thread assigning to this field. The contents of localDataObject may change while a thread is inside the critical section, allowing two threads to enter the critical section simultaneously.

The only other reference to localDataObject is the retrieval.

public Vector <Obj1, Obj2> getLocalDataObjects () 
{
    return localDataObject;
}

This isn't much different from how other objects are handled yet they don't return any Coverity issues.

In what scenario would a race condition occur?

How to resolve this Coverity error?

Unhandled Exception
  • 1,427
  • 14
  • 30
  • presumably (not coverity expert) the problem lies in localDataObject being an instance variable is open to assignment from other thread OUTSIDE the critical section which no synchronisation can catch. – Nikos M. Jul 01 '20 at 13:19
  • 2
    You should **always** use explicit lock objects (e.g. `private static final Object lock = new Object();`), not some general data objects to lock on. For more complex cases the `java.util.concurrent` package should be consulted. – Kayaman Jul 01 '20 at 13:26
  • @Kayaman, can you explain a bit why an explicit lock object resolves the issue? – Nikos M. Jul 01 '20 at 13:37
  • @NikosM. I didn't say anything about resolving the issue. Based on the little code I see, I don't think it should be solved with `synchronized` at all, but with something from `java.util.concurrent` depending on what the goal is. – Kayaman Jul 01 '20 at 13:47
  • @Kayaman, ok I was just trying to learn more about explicit locking and why general data objects are not good candidates for lock objects, as summarily as possible :) – Nikos M. Jul 01 '20 at 13:48
  • 1
    @NikosM. because concurrency is hard enough without trying to keep track of what objects are being used as locks either implictly or explicitly: [here](https://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java), [here](https://stackoverflow.com/questions/18356795/static-versus-non-static-lock-object-in-synchronized-block). When you have `synchronized(foo)`, but `foo` keeps changing, bugs arise when different threads synchronize (by accident) on different `foo` objects. – Kayaman Jul 01 '20 at 13:57
  • 2
    The code looks like caching. You return the local version, but at some point you'll update it with a remote version, and you want to keep things correct. However the choice of using the local data object as the lock is terrible, and frankly I'm a bit worried about the code I'm not seeing. If the only other reference to `localDataObject` is in the getter, then the code isn't fully correct (write is sync'd, but reads aren't). – Kayaman Jul 01 '20 at 14:04
  • 2
    @Kayaman replace “isn't fully correct” with “is entirely broken”. – Holger Jul 01 '20 at 17:16
  • 1
    @Holger you're right that would be the accurate term. The cache may appear to work at least under some conditions, but it would be impossible to realize it's not working by design, but as a side effect of some other code (e.g. the cases where `System.out.println` "fixes" [code](https://stackoverflow.com/questions/25425130/loop-doesnt-see-value-changed-by-other-thread-without-a-print-statement)). – Kayaman Jul 01 '20 at 17:24

0 Answers0