File: WaitNotInLoop.md

package info (click to toggle)
error-prone-java 2.18.0-1
  • links: PTS, VCS
  • area: main
  • in suites: bookworm, forky, sid, trixie
  • size: 23,204 kB
  • sloc: java: 222,992; xml: 1,319; sh: 25; makefile: 7
file content (166 lines) | stat: -rw-r--r-- 4,289 bytes parent folder | download | duplicates (2)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
`Object.wait()` is supposed to block until either another thread invokes the
`Object.notify()` or `Object.notifyAll()` method, or a specified amount of time
has elapsed. The various `Condition.await()` methods have similar behavior.
However, it is possible for a thread to wake up without either of those
occurring; these are called *spurious wakeups*.

Because of spurious wakeups, `Object.wait()` and `Condition.await()` must always
be called in a loop. The correct fix for this varies depending on what you are
trying to do.

## Wait until a condition becomes true

The incorrect code for this typically looks like:

Thread 1:

```java
synchronized (this) {
  if (!condition) {
    wait();
  }
  doStuffAssumingConditionIsTrue();
}
```

Thread 2:

```java
synchronized (this) {
  condition = true;
  notify();
}
```

If the call to `wait()` unblocks because of a spurious wakeup, then
`doStuffAssumingConditionIsTrue()` will be called even though `condition` is
still false. Instead of the `if`, you should use a `while`:

Thread 1:

```java
synchronized (this) {
  while (!condition) {
    wait();
  }
  doStuffAssumingConditionIsTrue();
}
```

This ensures that you only proceed to `doStuffAssumingConditionIsTrue()` if
`condition` is true. Note that the check of the condition variable must be
inside the synchronized block; otherwise you will have a race condition between
checking and setting the condition variable.

## Wait until an event occurs

The incorrect code for this typically looks like:

Thread 1:

```java
synchronized (this) {
  wait();
  doStuffAfterEvent();
}
```

Thread 2:

```java
// when event occurs
synchronized (this) {
  notify();
}
```

If the call to `wait()` unblocks because of a spurious wakeup, then
`doStuffAfterEvent()` will be called even though the event has not yet occurred.
You should rewrite this code so that the occurrence of the event sets a
condition variable as well as calls `notify()`, and the `wait()` is wrapped in a
while loop checking the condition variable. That is, it should look just like
[the previous example](#wait_until_a_condition_becomes_true).

## Wait until either a condition becomes true or a timeout occurs

The incorrect code for this typically looks like:

```java
synchronized (this) {
  if (!condition) {
    wait(timeout);
  }
  doStuffAssumingConditionIsTrueOrTimeoutHasOccurred();
}
```

A spurious wakeup could cause this to proceed to
`doStuffAssumingConditionIsTrueOrTimeoutHasOccurred()` even if the condition is
still false and time less than the timeout has elapsed. Instead, you should
write:

```java
synchronized (this) {
  long now = System.currentTimeMillis();
  long deadline = now + timeout;
  while (!condition && now < deadline) {
    wait(deadline - now);
    now = System.currentTimeMillis();
  }
  doStuffAssumingConditionIsTrueOrTimeoutHasOccurred();
}
```

## Wait for a fixed amount of time

First, a warning: This type of waiting/sleeping is often done when the real
intent is to wait until some operation completes, and then proceed. If that's
what you're trying to do, please consider rewriting your code to use one of the
patterns above. Otherwise you are depending on system-specific timing that
*will* change when you run on different machines.

The incorrect code for this typically looks like:

```java
synchronized (this) {
  // Give some time for the foos to bar
  wait(1000);
}
```

A spurious wakeup could cause this not to wait for a full 1000 ms. Instead, you
should use `Thread.sleep()`, which is not subject to spurious wakeups:

```java
Thread.sleep(1000);
```

## Wait forever

The incorrect code for this typically looks like:

```java
synchronized (this) {
  // wait forever
  wait();
}
```

A spurious wakeup could cause this not to wait forever. You should wrap the call
to `wait()` in a `while (true)` loop:

```java
synchronized (this) {
  // wait forever
  while (true) {
    wait();
  }
}
```

## More information

See Java Concurrency in Practice section 14.2.2, "Waking up too soon,"
[the Javadoc for `Object.wait()`](http://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#wait-long-),
and the "Implementation Considerations" section in
[the Javadoc for `Condition`](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Condition.html).