[YANGTOOLS-241] Remove double-checked locking in ReadOnlyTrieMap Created: 31/Jul/14 Updated: 10/Apr/22 Resolved: 04/Aug/14 |
|
| Status: | Resolved |
| Project: | yangtools |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | ||
| Reporter: | Tom Pantelis | Assignee: | Robert Varga |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Environment: |
Operating System: All |
||
| External issue ID: | 1460 |
| Priority: | High |
| Description |
|
ReadOnlyTrieMap uses the double-checked locking (DCL) technique in this method: protected Map<K, V> delegate() { } return readOnly; However DCL is not thread-safe. See http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html A few solutions: 1) The simplest solution is to avoid lazy initialization and created readOnly in 2) If we really want lazy initialization the remove the DCL: protected Map<K, V> delegate() { } return readOnly; The downside to synchronizing on 'this' is that users may also synchronize 3) Keep the lazy initialization and DCL but make readOnly volatile. However this DCL is usually not worth the trouble. Depending on usage, ie we can assume users will call delegate most of the time, I would opt for #1. Otherwise #2. |
| Comments |
| Comment by Robert Varga [ 01/Aug/14 ] |
|
Valid comment, although the bad effects of possible compiler optimization are limited to increased cpu/memory usage. Taking the snapshot affects performance on subsequent operations on the backing map, so we do not want to instantiate it unless absolutely necessary. DCL is needed because accessing the snapshot is on perf-critical path, so we want to elide locking as much as possible. Finally this is a class which has a lot of instances, so an internal lock has prohibitive cost, especially since none of the current users synchronize on these objects. The data in the snapshot is logically immutable, so option #3 it is: https://git.opendaylight.org/gerrit/#/c/9567/ |
| Comment by Tom Pantelis [ 01/Aug/14 ] |
|
That article I linked to says volatile may not fix it but that contradicts other articles I've read that say volatile does fix it (e.g. http://jeremymanson.blogspot.com/2008/05/double-checked-locking.html). I turns out that article is older (looks like from 2001) prior to java 1.5 where volatile's semantics were vague. With the revised, clearly-defined semantics for volatile in the newer 1.5 JMM, volatile is now safe to use for DCL (in fact I remember reading that they specifically defined volatile semantics to make DCL work). |
| Comment by Tom Pantelis [ 01/Aug/14 ] |
|
Ok - the semantic they added in JMM 1.5 that makes volatile safe for DCL is that non-volatile writes (and non-final) occurring before a volatile (or final) write cannot be re-ordered after it (although non-volatile writes occurring after can be re-ordered before it). Same for reads. So, for DCL, any non-volatile and non-final writes in the constructor are guaranteed to be visible to all threads b/c they cannot be moved after the volatile write of the DCL variable. (In reply to Tom Pantelis from comment #2) |