Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/136#discussion_r64989703
--- Diff:
software/base/src/main/java/org/apache/brooklyn/entity/software/base/InboundPortsUtils.java
---
@@ -77,12 +82,17 @@
Set<ConfigKey<?>> configKeys =
Sets.newHashSet(extraConfigKeys);
configKeys.addAll(entity.getEntityType().getConfigKeys());
// Also add dynamically added config keys
-
configKeys.addAll(((EntityInternal)entity).config().getBag().getAllConfigAsConfigKeyMap().keySet());
+ Map<ConfigKey<?>, ?> configuredConfigKeys =
ImmutableMap.copyOf(((EntityInternal)
entity).config().getBag().getAllConfigAsConfigKeyMap());
+ configKeys.addAll(configuredConfigKeys.keySet());
--- End diff --
@kiuby88 I didn't follow your description of the problem: "but I found an
exception when this tests was executed, because the entity was accessed by
another thread (I found and InterruptedException what threw a NPE. "
Was it an `InterruptedException`? Those really are special cases - just let
your method terminate, propagating the exception if you are interrupted. Or was
it a NullPointerException, or perhaps a ConcurrentModificationException? The
call to `getAllConfigAsConfigKeyMap()` will give you a copy. So you should be
safe to use that directly, without taking another copy.
---
In terms of the low-level comment about use of `ImmutableMap`...
Alex is right: we need to be careful about where we use `ImmutableMap`
because it will throw a `NullPointerException` if any key or value is null. In
our config, a user might explicitly have set a config value to null (e.g. to
override the default). Therefore we should not try to copy
`getAllConfigAsConfigKeyMap()` as an immutable map.
I do really love guava's `ImmutableMap`. I think it's a smart move to fail
on null in many many cases. Unfortunately, because of the way nulls are used in
various parts of Brooklyn (in particular config and sensors), we can't do it
here.
You could change this to `MutableMap.copyOf(...)`. I agree that you don't
want to modify the copy, but you are also not sharing it so it's ok if you
leave it mutable. (we do not have a `ImmutableMapAllowingNulls`, so let's just
make do with `MutableMap`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
