matthiasblaesing commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1367879032
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
Testing does not help when reasoning about possible synchronisation issues.
They have the tendency not to manifest in tests and show in real life. The
argument must be based on what is guaranteed by the language.
With this change the following can happen:
1. Call to `LogViewMgr#getInstance` with a fixed URI `uri1` happens. There
is no key for `uri1` in the map `instances` and this the supplier is invoked.
It creates a new `LogViewMgr`, wraps it into a `WeakReference`, places it in
`instances` and returns it to the caller, which unwraps it using
`WeakReference#get` and returns it.
2. The system does something with the `LogViewMgr` instance and at some
point removes the last hard reference to it
3. Garbage collections happens
4. `LogViewMgr#getInstance` with a fixed URI `uri1` is called again. Now
there is a `WeakReference` for the key `uri1`, which is returned and
`WeakReference#get` is invoked on that. But because (3) happend you now get
`null`. The code is broken.
There is another race in step 1. To better see this, the code can be split
to:
```java
public static LogViewMgr getInstance(String uri) {
WeakRef<LogViewMgr> logViewMgrRef = instances.computeIfAbsent(uri,
key ->
new WeakReference<LogViewMgr>(new LogViewMgr(uri))
);
LogViewMgr logViewMgr = logViewMgrRef.get();
return logViewMgr;
}
```
There is a very short window where the instance created in the supplier is
not hard referenced anymore. After it is created and before
`logViewMgrRef.get()` it is possible, that GC happens and the instance is
collected. `logViewMgrRef.get()` will then yield `null`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
