Skip to content

Commit 652e3bd

Browse files
authored
Fix unsafe Random use in ReaderWriterLock samples (#4099)
The ReaderWriterLock samples were sharing a single Random instance across threads. The various .Next methods on Random instances are not safe to be called concurrently. Now, each thread has its own Random instance that it uses. Some of the worker methods needed to be adjusted to take a Random instance as input. I considered using a thread-local Random instance. However, the sample is already complex, so I opted to avoid introducing thread-local values, which would need to be briefly explained.
1 parent 85e4baa commit 652e3bd

File tree

3 files changed

+61
-60
lines changed
  • samples/snippets
    • cpp/VS_Snippets_CLR_System/system.Threading.ReaderWriterLock/CPP
    • csharp/VS_Snippets_CLR_System/system.Threading.ReaderWriterLock/CS
    • visualbasic/VS_Snippets_CLR_System/system.Threading.ReaderWriterLock/VB

3 files changed

+61
-60
lines changed

samples/snippets/cpp/VS_Snippets_CLR_System/system.Threading.ReaderWriterLock/CPP/source.cpp

Lines changed: 44 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
21
//<Snippet1>
32
// This example shows a ReaderWriterLock protecting a shared
43
// resource that is read concurrently and written exclusively
@@ -23,7 +22,6 @@ public ref class Test
2322
//</Snippet2>
2423
literal int numThreads = 26;
2524
static bool running = true;
26-
static Random^ rnd = gcnew Random;
2725

2826
// Statistics.
2927
static int readerTimeouts = 0;
@@ -32,10 +30,11 @@ public ref class Test
3230
static int writes = 0;
3331
static void ThreadProc()
3432
{
35-
33+
Random^ rnd = gcnew Random;
34+
3635
// As long as a thread runs, it randomly selects
37-
// various ways to read and write from the shared
38-
// resource. Each of the methods demonstrates one
36+
// various ways to read and write from the shared
37+
// resource. Each of the methods demonstrates one
3938
// or more features of ReaderWriterLock.
4039
while ( running )
4140
{
@@ -44,12 +43,12 @@ public ref class Test
4443
ReadFromResource( 10 );
4544
else
4645
if ( action < .81 )
47-
ReleaseRestore( 50 );
46+
ReleaseRestore( rnd, 50 );
4847
else
4948
if ( action < .90 )
50-
UpgradeDowngrade( 100 );
49+
UpgradeDowngrade( rnd, 100 );
5150
else
52-
WriteToResource( 100 );
51+
WriteToResource( rnd, 100 );
5352
}
5453
}
5554

@@ -64,23 +63,23 @@ public ref class Test
6463
rwl->AcquireReaderLock( timeOut );
6564
try
6665
{
67-
66+
6867
// It is safe for this thread to read from
6968
// the shared resource.
7069
Display( String::Format( "reads resource value {0}", resource ) );
7170
Interlocked::Increment( reads );
7271
}
7372
finally
7473
{
75-
74+
7675
// Ensure that the lock is released.
7776
rwl->ReleaseReaderLock();
7877
}
7978

8079
}
81-
catch ( ApplicationException^ )
80+
catch ( ApplicationException^ )
8281
{
83-
82+
8483
// The reader lock request timed out.
8584
Interlocked::Increment( readerTimeouts );
8685
}
@@ -92,14 +91,14 @@ public ref class Test
9291
//<Snippet4>
9392
// Shows how to request and release the writer lock, and
9493
// how to handle time-outs.
95-
static void WriteToResource( int timeOut )
94+
static void WriteToResource( Random^ rnd, int timeOut )
9695
{
9796
try
9897
{
9998
rwl->AcquireWriterLock( timeOut );
10099
try
101100
{
102-
101+
103102
// It is safe for this thread to read or write
104103
// from the shared resource.
105104
resource = rnd->Next( 500 );
@@ -108,15 +107,15 @@ public ref class Test
108107
}
109108
finally
110109
{
111-
110+
112111
// Ensure that the lock is released.
113112
rwl->ReleaseWriterLock();
114113
}
115114

116115
}
117-
catch ( ApplicationException^ )
116+
catch ( ApplicationException^ )
118117
{
119-
118+
120119
// The writer lock request timed out.
121120
Interlocked::Increment( writerTimeouts );
122121
}
@@ -129,32 +128,32 @@ public ref class Test
129128
// Shows how to request a reader lock, upgrade the
130129
// reader lock to the writer lock, and downgrade to a
131130
// reader lock again.
132-
static void UpgradeDowngrade( int timeOut )
131+
static void UpgradeDowngrade( Random^ rnd, int timeOut )
133132
{
134133
try
135134
{
136135
rwl->AcquireReaderLock( timeOut );
137136
try
138137
{
139-
138+
140139
// It is safe for this thread to read from
141140
// the shared resource.
142141
Display( String::Format( "reads resource value {0}", resource ) );
143142
Interlocked::Increment( reads );
144-
143+
145144
// If it is necessary to write to the resource,
146-
// you must either release the reader lock and
145+
// you must either release the reader lock and
147146
// then request the writer lock, or upgrade the
148147
// reader lock. Note that upgrading the reader lock
149148
// puts the thread in the write queue, behind any
150-
// other threads that might be waiting for the
149+
// other threads that might be waiting for the
151150
// writer lock.
152151
try
153152
{
154153
LockCookie lc = rwl->UpgradeToWriterLock( timeOut );
155154
try
156155
{
157-
156+
158157
// It is safe for this thread to read or write
159158
// from the shared resource.
160159
resource = rnd->Next( 500 );
@@ -163,36 +162,36 @@ public ref class Test
163162
}
164163
finally
165164
{
166-
165+
167166
// Ensure that the lock is released.
168167
rwl->DowngradeFromWriterLock( lc );
169168
}
170169

171170
}
172-
catch ( ApplicationException^ )
171+
catch ( ApplicationException^ )
173172
{
174-
173+
175174
// The upgrade request timed out.
176175
Interlocked::Increment( writerTimeouts );
177176
}
178177

179-
180-
// When the lock has been downgraded, it is
178+
179+
// When the lock has been downgraded, it is
181180
// still safe to read from the resource.
182181
Display( String::Format( "reads resource value {0}", resource ) );
183182
Interlocked::Increment( reads );
184183
}
185184
finally
186185
{
187-
186+
188187
// Ensure that the lock is released.
189188
rwl->ReleaseReaderLock();
190189
}
191190

192191
}
193-
catch ( ApplicationException^ )
192+
catch ( ApplicationException^ )
194193
{
195-
194+
196195
// The reader lock request timed out.
197196
Interlocked::Increment( readerTimeouts );
198197
}
@@ -207,37 +206,37 @@ public ref class Test
207206
// to determine whether another thread has obtained
208207
// a writer lock since this thread last accessed the
209208
// resource.
210-
static void ReleaseRestore( int timeOut )
209+
static void ReleaseRestore( Random^ rnd, int timeOut )
211210
{
212211
int lastWriter;
213212
try
214213
{
215214
rwl->AcquireReaderLock( timeOut );
216215
try
217216
{
218-
217+
219218
// It is safe for this thread to read from
220219
// the shared resource. Cache the value. (You
221220
// might do this if reading the resource is
222221
// an expensive operation.)
223222
int resourceValue = resource;
224223
Display( String::Format( "reads resource value {0}", resourceValue ) );
225224
Interlocked::Increment( reads );
226-
225+
227226
// Save the current writer sequence number.
228227
lastWriter = rwl->WriterSeqNum;
229-
228+
230229
// Release the lock, and save a cookie so the
231230
// lock can be restored later.
232231
LockCookie lc = rwl->ReleaseLock();
233-
234-
// Wait for a random interval (up to a
232+
233+
// Wait for a random interval (up to a
235234
// quarter of a second), and then restore
236235
// the previous state of the lock. Note that
237236
// there is no timeout on the Restore method.
238237
Thread::Sleep( rnd->Next( 250 ) );
239238
rwl->RestoreLock( lc );
240-
239+
241240
// Check whether other threads obtained the
242241
// writer lock in the interval. If not, then
243242
// the cached value of the resource is still
@@ -255,15 +254,15 @@ public ref class Test
255254
}
256255
finally
257256
{
258-
257+
259258
// Ensure that the lock is released.
260259
rwl->ReleaseReaderLock();
261260
}
262261

263262
}
264-
catch ( ApplicationException^ )
263+
catch ( ApplicationException^ )
265264
{
266-
265+
267266
// The reader lock request timed out.
268267
Interlocked::Increment( readerTimeouts );
269268
}
@@ -273,7 +272,7 @@ public ref class Test
273272

274273
//</Snippet6>
275274
// Helper method briefly displays the most recent
276-
// thread action. Comment out calls to Display to
275+
// thread action. Comment out calls to Display to
277276
// get a better idea of throughput.
278277
static void Display( String^ msg )
279278
{
@@ -288,7 +287,7 @@ public ref class Test
288287
int main()
289288
{
290289
array<String^>^args = Environment::GetCommandLineArgs();
291-
290+
292291
// Start a series of threads. Each thread randomly
293292
// performs reads and writes on the shared resource.
294293
array<Thread^>^t = gcnew array<Thread^>(Test::numThreads);
@@ -301,7 +300,7 @@ int main()
301300
Thread::Sleep( 300 );
302301

303302
}
304-
303+
305304
// Tell the threads to shut down, then wait until they all
306305
// finish.
307306
Test::running = false;
@@ -310,7 +309,7 @@ int main()
310309
t[ i ]->Join();
311310

312311
}
313-
312+
314313
// Display statistics.
315314
Console::WriteLine( "\r\n {0} reads, {1} writes, {2} reader time-outs, {3} writer time-outs.", Test::reads, Test::writes, Test::readerTimeouts, Test::writerTimeouts );
316315
Console::WriteLine( "Press ENTER to exit." );

samples/snippets/csharp/VS_Snippets_CLR_System/system.Threading.ReaderWriterLock/CS/source.cs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ public class Example
1313

1414
const int numThreads = 26;
1515
static bool running = true;
16-
static Random rnd = new Random();
1716

1817
// Statistics.
1918
static int readerTimeouts = 0;
@@ -48,18 +47,20 @@ public static void Main()
4847

4948
static void ThreadProc()
5049
{
50+
Random rnd = new Random();
51+
5152
// Randomly select a way for the thread to read and write from the shared
5253
// resource.
5354
while (running) {
5455
double action = rnd.NextDouble();
5556
if (action < .8)
5657
ReadFromResource(10);
5758
else if (action < .81)
58-
ReleaseRestore(50);
59+
ReleaseRestore(rnd, 50);
5960
else if (action < .90)
60-
UpgradeDowngrade(100);
61+
UpgradeDowngrade(rnd, 100);
6162
else
62-
WriteToResource(100);
63+
WriteToResource(rnd, 100);
6364
}
6465
}
6566

@@ -88,7 +89,7 @@ static void ReadFromResource(int timeOut)
8889

8990
//<Snippet4>
9091
// Request and release the writer lock, and handle time-outs.
91-
static void WriteToResource(int timeOut)
92+
static void WriteToResource(Random rnd, int timeOut)
9293
{
9394
try {
9495
rwl.AcquireWriterLock(timeOut);
@@ -113,7 +114,7 @@ static void WriteToResource(int timeOut)
113114
//<Snippet5>
114115
// Requests a reader lock, upgrades the reader lock to the writer
115116
// lock, and downgrades it to a reader lock again.
116-
static void UpgradeDowngrade(int timeOut)
117+
static void UpgradeDowngrade(Random rnd, int timeOut)
117118
{
118119
try {
119120
rwl.AcquireReaderLock(timeOut);
@@ -164,7 +165,7 @@ static void UpgradeDowngrade(int timeOut)
164165
// Release all locks and later restores the lock state.
165166
// Uses sequence numbers to determine whether another thread has
166167
// obtained a writer lock since this thread last accessed the resource.
167-
static void ReleaseRestore(int timeOut)
168+
static void ReleaseRestore(Random rnd, int timeOut)
168169
{
169170
int lastWriter;
170171

0 commit comments

Comments
 (0)