Possible issues w/ PropertyWrapper?

[pkwarren]

When looking at the implementation of PropertyWrapper, I've seen two potential issues for consumers of this API:

• PropertyWrapper wraps a DynamicProperty (which is thread safe), but appears to not be thread safe itself (specifically the callbackList field). It would be useful to document this (that addCallback and removeAllCallbacks should be synchronized) or perhaps switch the callbackList to a CopyOnWriteArrayList similar to DynamicProperty. Reference:

  archaius/archaius-core/src/main/java/com/netflix/config/PropertyWrapper.java

  Line 41 in e623c01

  private final List<Runnable> callbackList = Lists.newArrayList();

• In removeAllCallbacks, the callback is removed from DynamicProperty but the entry in callbackList is not removed. This will lead to a resource leak over time as there doesn't appear to be a way to remove callbacks from callbackList. Reference:

  archaius/archaius-core/src/main/java/com/netflix/config/PropertyWrapper.java

  Lines 156 to 164 in e623c01

  	/**
  	* Remove all callbacks registered through this instance of property
  	*/
  	@Override
  	public void removeAllCallbacks() {
  	for (Runnable callback: callbackList) {
  	prop.removeCallback(callback);
  	}
  	}

I can raise a PR to resolve these issues.

[howardyuan]

@pkwarren Sure, please raise a PR so we can look at it together. Thanks.

[pkwarren]

@howardyuan - Opened #470 with a proposed fix for these issues.

[pkwarren]

@howardyuan - Have you had a chance to look at this PR?

[rgallardo-netflix]

won't fix: v1 has been deprecated