shannah / Java-Objective-C-Bridge

A thin bridge that allows for two-way communication from Java to Objective-C.
123 stars 25 forks source link

Client.getInstance() and getRawClient() do not perform safe lazy initialization #25

Closed Marcono1234 closed 3 years ago

Marcono1234 commented 3 years ago

The static methods Client.getInstance() and getRawClient() do not perform safe lazy initialization: https://github.com/shannah/Java-Objective-C-Bridge/blob/473221aeef8f12ca9a659d899f3ca7092aed27ef/src/main/java/ca/weblite/objc/Client.java#L46-L51

To correct this perform these steps:

  1. Make instance and rawClient volatile
  2. In the access methods, do double checked locking:
    1. Check if instance == null (for increased performance store current value in local var)
    2. If null:
      1. Synchronize
      2. Check again if instance == null (and overwrite local var, see 2.1)
      3. If null: Initialize (and update local var, see 2.1)
    3. Return instance (or local var) value

E.g.:

private static volatile Client instance;

public Client getInstance() {
    Client localInstance = instance;
    if (localInstance == null) {
        // Could also use dedicated lock for `instance` and `rawClient`, but likely not worth it
        synchronized (Client.class) {
            localInstance = instance;
            if (localInstance == null) {
                instance = localInstance = new Client();
            }
        }
    }
    return localInstance;
}

Or alternatively making these methods synchronized would also suffice and the performance decrease (if any) would likely be negligible. See also "LCK10-J. Use a correct form of the double-checked locking idiom"

For sanity I would also recommend:

Marcono1234 commented 3 years ago

@shannah thanks! Sorry I overlooked that there were setters for coerceInputs and coerceOutputs. Hopefully this change does not break existing code which only set one of them to false.

I have commented on your commit, could you please have a look?

Edit: Sorry, had another look at it and maybe the lazy initialization is not worth it in the first place, I have omitted it in #35.