tjwatson commented on a change in pull request #81:
URL: https://github.com/apache/felix-dev/pull/81#discussion_r674753430
##########
File path:
scr/src/main/java/org/apache/felix/scr/impl/inject/field/FieldHandler.java
##########
@@ -171,9 +171,9 @@ private boolean initField(final Object componentInstance,
return objects;
}
- private MethodResult updateField(final METHOD_TYPE mType,
- final Object componentInstance,
- final BindParameters bp)
+ private synchronized MethodResult updateField(final METHOD_TYPE mType,
Review comment:
I think this synchronization is too broad. In particular I am concerned
about synchronization when making the calls to
`org.apache.felix.scr.impl.inject.ValueUtils.getValue(String, ValueType,
Class<?>, ScrComponentContext, RefPair<?, ?>)` which ultimately can set off a
chain of getService calls that may trigger activating dependency components.
A more safe fix is to use a synchronized map and synchronize on the map
around the bit of code that needs it:
```java
Map<RefPair<?, ?>, Object> boundValues =
bp.getComponentContext().getBoundValues(metadata.getName());
synchronized (boundValues)
{
if ( this.metadata.isOptional() &&
!this.metadata.isStatic() )
{
// we only reset if it was previously set with this
value
if ( boundValues.size() == 1 )
{
this.setFieldValue(componentInstance, null);
}
}
boundValues.remove(refPair);
}
```
Then I would just do `Collections.synchronizedMap(new TreeMap<>(...` when
creating the map for bound values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
