-
Notifications
You must be signed in to change notification settings - Fork 96
Fixed the synchronization strategy in ExecutorServiceProvider #1470
Conversation
this seems to break a lot of tests |
Wow. Compiler bug? ASM bug? Both? |
Could this failure be related to Guice 3.0 is incompatible with Java 8? See #393 |
But the stacktrace there is really similar:
|
error message when tested with guice 4
|
ConcurrentHashMap does not allow null as keys |
*/ | ||
@Singleton | ||
public class ExecutorServiceProvider implements Provider<ExecutorService>, IDisposable { | ||
|
||
private final Map<String, ExecutorService> instanceCache = new ConcurrentHashMap<>(3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that ConcurrentHashMap does not allow null-keys or null-values, it may also be ok to use Collections.synchronizedMap here, since I don't expect a lot of pressure on the lookup of an ExecutorService.
get(String key)
should be rewritten to use computeIfAbsent(..)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispose
must sync on the map, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get(String key) should be rewritten to use
computeIfAbsent(..)
.
I can think of two ways; however, I am not convinced by either. In the first one, calling computeIfAbsent(...)
inside the if (result == null)
doesn't make much sense. We already know that the value is null
before calling computeIfAbsent(...)
. The second one would block threads even if the map has a non-null value for the key requested by the second thread.
@szarnekow Please suggest if there is any other way to rewrite this method.
ExecutorService result = instanceCache.get(key);
if (result == null) {
synchronized (instanceCache) {
result = instanceCache.computeIfAbsent(key, k -> createInstance(k));
}
}
return result;
synchronized (instanceCache) {
return instanceCache.computeIfAbsent(key, k -> createInstance(k));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steps:
- Use a
Collections.synchronizedMap(new HashMap<>())
for the field initializer - Rewrite and simplify get(String) towards
public ExecutorService get(String key) { return instanceCache.computeIfAbsent(key, this::createInstance)); }
- fix dispose to sync on the instanceCache, too:
public void dipose() { synchronized(instanceCache) { .. } }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szarnekow computeIfAbsent(...)
is not atomic. Don't we have to sync on the map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SynchronizedMap.computeIfAbsent(K, Function<? super K, ? extends V>)
is safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, tx for clarification.
Signed-off-by: nbhusare <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you, @nbhusare! |
PS #1442 (comment) for details regarding this fix.
Signed-off-by: nbhusare [email protected]