• Home
  • Line#
  • Scopes#
  • Navigate#
  • Raw
  • Download
1# Forwarding and `ReadWriteLock`
2
3This document explains the approach chosen to implement
4[forwarding](https://en.wikipedia.org/wiki/Forwarding_(object-oriented_programming))
5protected with a `ReadWriteLock`.
6
7A `ReadWriteLock` is used rather than `synchronized` blocks to the limit
8opportunitie for stutter on the UI thread when waiting for this shared resource.
9
10For context see
11`base/android/java/src/org/chromium/base/metrics/CachingUmaRecorder.java`
12
13### Method A
14
15```java
16private final ReentrantReadWriteLock mRwLock = new ReentrantReadWriteLock();
17
18@Nullable
19private UmaRecorder mDelegate;
20
21@Override
22public void record(Sample sample) {
23    mRwLock.readLock().lock();
24    try {
25        if (mDelegate != null) {
26            mDelegate.record(sample);  // Called with a read lock
27            return;
28        }
29    } finally {
30        mRwLock.readLock().unlock();
31    }
32
33    mRwLock.writeLock().lock();
34    try {
35        if (mDelegate == null) {
36            this.cache(sample);
37            return; // Skip the lock downgrade.
38        }
39        // Downgrade by acquiring read lock before releasing write lock
40        mRwLock.readLock().lock();
41    } finally {
42        mRwLock.writeLock().unlock();
43    }
44
45    // Downgraded to read lock.
46    try {
47        assert mDelegate != null;
48        mDelegate.record(sample);  // Called with a read lock
49    } finally {
50        mRwLock.readLock().unlock();
51    }
52}
53```
54
55### Method B
56
57```java
58private final ReentrantReadWriteLock mRwLock = new ReentrantReadWriteLock();
59
60@Nullable
61private UmaRecorder mDelegate;
62
63@Override
64public void record(Sample sample) {
65    mRwLock.readLock().lock();
66    try {
67        if (mDelegate != null) {
68            mDelegate.record(sample);  // Called with a read lock
69            return;
70        }
71    } finally {
72        mRwLock.readLock().unlock();
73    }
74
75    mRwLock.writeLock().lock();
76    try {
77        if (mDelegate == null) {
78            this.cache(sample);
79        } else {
80            mDelegate.record(sample);  // Called with a *write* lock
81        }
82    } finally {
83        mRwLock.writeLock().unlock();
84    }
85}
86```
87
88## Reasoning
89
90Code of method B is visibly and conceptually simpler, since it doesn't involve
91lock downgrading. However:
92
93 *  Method B invokes `record(Sample)` with one of two available locks. The two
94    locks are difficult to distinguish in a stack trace.
95 *  Method A always uses the read lock which results in a less complex
96    interaction with `mDelegate`.
97
98Even if we ask every implementation of `UmaRecord#record(Sample)` to not block
99and run quickly, bugs happen.
100
101### Hypothetical example
102
103An invocation of `UmaRecord#record(Sample)` under some circumstances interacts
104with the same instance of `CachingUmaRecorder`. We don't want this to ever
105happen, so this is a bug. The current delegate implementation crosses JNI and
106calls into metrics code that should terminate without crossing back to Java.
107
108This bug results in the following scenarios:
109
110| # | `CUR` holds  | `mDelegate` uses | thread    | result   |
111|---|--------------|------------------|-----------|----------|
112| 1 | `readLock`   | `readLock`       | same      | OK       |
113| 2 | `readLock`   | `writeLock`      | same      | deadlock |
114| 3 | `readLock`   | `readLock`       | different | OK       |
115| 4 | `readLock`   | `writeLock`      | different | blocks   |
116| 5 | `writeLock`  | `readLock`       | same      | OK       |
117| 6 | `writeLock`  | `writeLock`      | same      | OK       |
118| 7 | `writeLock`  | `readLock`       | different | blocks   |
119| 8 | `writeLock`  | `writeLock`      | different | blocks   |
120
121Method A eliminates the possibility of scenarios 5-8.
122