-
Notifications
You must be signed in to change notification settings - Fork 5
Expand file tree
/
Copy patha_million_ways_to_data_race_in_go.html
More file actions
787 lines (764 loc) · 71.6 KB
/
a_million_ways_to_data_race_in_go.html
File metadata and controls
787 lines (764 loc) · 71.6 KB
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
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725
726
727
728
729
730
731
732
733
734
735
736
737
738
739
740
741
742
743
744
745
746
747
748
749
750
751
752
753
754
755
756
757
758
759
760
761
762
763
764
765
766
767
768
769
770
771
772
773
774
775
776
777
778
779
780
781
782
783
784
785
786
787
<!--
This file has been auto-generated by main.rs from a markdown file of the same name.
Do not edit it by hand.
-->
<!DOCTYPE html>
<html>
<head>
<title>A million ways to die from a data race in Go</title>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<link type="application/atom+xml" href="/blog/feed.xml" rel="self">
<link rel="shortcut icon" type="image/ico" href="/blog/favicon.ico">
<link rel="stylesheet" type="text/css" href="main.css">
<link rel="stylesheet" href="https://unpkg.com/@highlightjs/cdn-assets@11.8.0/styles/default.min.css">
<script type="module" src="main.js" async></script>
<script type="module" src="search.js" async></script>
</head>
<body>
<div id="banner">
<div id="name">
<img id="me" src="me.jpeg">
<span>Philippe Gaultier</span>
</div>
<input id="search" placeholder="🔎 Search" autocomplete=off>
<ul>
<li> <button id="dark-light-mode">Dark/Light</button> </li>
<li> <a href="/blog/body_of_work.html">Body of work</a> </li>
<li> <a href="/blog/articles-by-tag.html">Tags</a> </li>
<li> <a href="https://github.com/gaultier/resume/raw/master/Philippe_Gaultier_resume_en.pdf">
Resume
</a> </li>
<li> <a href="/blog/feed.xml">
<svg viewBox="0 0 24 24" fill="currentColor" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M5.5 3.5C4.39543 3.5 3.5 4.39543 3.5 5.5V18.5C3.5 19.6046 4.39543 20.5 5.5 20.5H18.5C19.6046 20.5 20.5 19.6046 20.5 18.5V5.5C20.5 4.39543 19.6046 3.5 18.5 3.5H5.5ZM7 19C8.10457 19 9 18.1046 9 17C9 15.8954 8.10457 15 7 15C5.89543 15 5 15.8954 5 17C5 18.1046 5.89543 19 7 19ZM6.14863 10.5052C6.14863 10.0379 6.52746 9.65906 6.99478 9.65906C7.95949 9.65906 8.91476 9.84908 9.80603 10.2183C10.6973 10.5874 11.5071 11.1285 12.1893 11.8107C12.8715 12.4929 13.4126 13.3027 13.7817 14.194C14.1509 15.0852 14.3409 16.0405 14.3409 17.0052C14.3409 17.4725 13.9621 17.8514 13.4948 17.8514C13.0275 17.8514 12.6486 17.4725 12.6486 17.0052C12.6486 16.2627 12.5024 15.5275 12.2183 14.8416C11.9341 14.1556 11.5177 13.5324 10.9927 13.0073C10.4676 12.4823 9.84437 12.0659 9.15842 11.7817C8.47246 11.4976 7.73726 11.3514 6.99478 11.3514C6.52746 11.3514 6.14863 10.9725 6.14863 10.5052ZM7 5.15385C6.53268 5.15385 6.15385 5.53268 6.15385 6C6.15385 6.46732 6.53268 6.84615 7 6.84615C8.33342 6.84615 9.65379 7.10879 10.8857 7.61907C12.1176 8.12935 13.237 8.87728 14.1799 9.82015C15.1227 10.763 15.8707 11.8824 16.3809 13.1143C16.8912 14.3462 17.1538 15.6666 17.1538 17C17.1538 17.4673 17.5327 17.8462 18 17.8462C18.4673 17.8462 18.8462 17.4673 18.8462 17C18.8462 15.4443 18.5397 13.9039 17.9444 12.4667C17.3491 11.0294 16.4765 9.72352 15.3765 8.6235C14.2765 7.52349 12.9706 6.65091 11.5333 6.05558C10.0961 5.46026 8.55566 5.15385 7 5.15385Z" fill="currentColor"/>
</svg>
</a> </li>
<li> <a href="https://www.linkedin.com/in/philippegaultier/">
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" data-supported-dps="24x24" fill="currentColor" width="24" height="24" focusable="false">
<path d="M20.5 2h-17A1.5 1.5 0 002 3.5v17A1.5 1.5 0 003.5 22h17a1.5 1.5 0 001.5-1.5v-17A1.5 1.5 0 0020.5 2zM8 19H5v-9h3zM6.5 8.25A1.75 1.75 0 118.3 6.5a1.78 1.78 0 01-1.8 1.75zM19 19h-3v-4.74c0-1.42-.6-1.93-1.38-1.93A1.74 1.74 0 0013 14.19a.66.66 0 000 .14V19h-3v-9h2.9v1.3a3.11 3.11 0 012.7-1.4c1.55 0 3.36.86 3.36 3.66z"/>
</svg>
</a> </li>
<li> <a href="https://github.com/gaultier">
<svg height="32" aria-hidden="true" viewBox="0 0 24 24" version="1.1" width="32" data-view-component="true" fill="currentColor">
<path d="M12.5.75C6.146.75 1 5.896 1 12.25c0 5.089 3.292 9.387 7.863 10.91.575.101.79-.244.79-.546 0-.273-.014-1.178-.014-2.142-2.889.532-3.636-.704-3.866-1.35-.13-.331-.69-1.352-1.18-1.625-.402-.216-.977-.748-.014-.762.906-.014 1.553.834 1.769 1.179 1.035 1.74 2.688 1.25 3.349.948.1-.747.402-1.25.733-1.538-2.559-.287-5.232-1.279-5.232-5.678 0-1.25.445-2.285 1.178-3.09-.115-.288-.517-1.467.115-3.048 0 0 .963-.302 3.163 1.179.92-.259 1.897-.388 2.875-.388.977 0 1.955.13 2.875.388 2.2-1.495 3.162-1.179 3.162-1.179.633 1.581.23 2.76.115 3.048.733.805 1.179 1.825 1.179 3.09 0 4.413-2.688 5.39-5.247 5.678.417.36.776 1.05.776 2.128 0 1.538-.014 2.774-.014 3.162 0 .302.216.662.79.547C20.709 21.637 24 17.324 24 12.25 24 5.896 18.854.75 12.5.75Z"/>
</svg>
</a> </li>
<li> <a href="https://hachyderm.io/@pg">
<svg width="75" height="79" viewBox="0 0 75 79" xmlns="http://www.w3.org/2000/svg" fill="currentColor">
<path d="M73.8393 17.4898C72.6973 9.00165 65.2994 2.31235 56.5296 1.01614C55.05 0.797115 49.4441 0 36.4582 0H36.3612C23.3717 0 20.585 0.797115 19.1054 1.01614C10.5798 2.27644 2.79399 8.28712 0.904997 16.8758C-0.00358524 21.1056 -0.100549 25.7949 0.0682394 30.0965C0.308852 36.2651 0.355538 42.423 0.91577 48.5665C1.30307 52.6474 1.97872 56.6957 2.93763 60.6812C4.73325 68.042 12.0019 74.1676 19.1233 76.6666C26.7478 79.2728 34.9474 79.7055 42.8039 77.9162C43.6682 77.7151 44.5217 77.4817 45.3645 77.216C47.275 76.6092 49.5123 75.9305 51.1571 74.7385C51.1797 74.7217 51.1982 74.7001 51.2112 74.6753C51.2243 74.6504 51.2316 74.6229 51.2325 74.5948V68.6416C51.2321 68.6154 51.2259 68.5896 51.2142 68.5661C51.2025 68.5426 51.1858 68.522 51.1651 68.5058C51.1444 68.4896 51.1204 68.4783 51.0948 68.4726C51.0692 68.4669 51.0426 68.467 51.0171 68.4729C45.9835 69.675 40.8254 70.2777 35.6502 70.2682C26.7439 70.2682 24.3486 66.042 23.6626 64.2826C23.1113 62.762 22.7612 61.1759 22.6212 59.5646C22.6197 59.5375 22.6247 59.5105 22.6357 59.4857C22.6466 59.4609 22.6633 59.4391 22.6843 59.422C22.7053 59.4048 22.73 59.3929 22.7565 59.3871C22.783 59.3813 22.8104 59.3818 22.8367 59.3886C27.7864 60.5826 32.8604 61.1853 37.9522 61.1839C39.1768 61.1839 40.3978 61.1839 41.6224 61.1516C46.7435 61.008 52.1411 60.7459 57.1796 59.7621C57.3053 59.7369 57.431 59.7154 57.5387 59.6831C65.4861 58.157 73.0493 53.3672 73.8178 41.2381C73.8465 40.7606 73.9184 36.2364 73.9184 35.7409C73.9219 34.0569 74.4606 23.7949 73.8393 17.4898Z"/>
<path d="M61.2484 27.0263V48.114H52.8916V27.6475C52.8916 23.3388 51.096 21.1413 47.4437 21.1413C43.4287 21.1413 41.4177 23.7409 41.4177 28.8755V40.0782H33.1111V28.8755C33.1111 23.7409 31.0965 21.1413 27.0815 21.1413C23.4507 21.1413 21.6371 23.3388 21.6371 27.6475V48.114H13.2839V27.0263C13.2839 22.7176 14.384 19.2946 16.5843 16.7572C18.8539 14.2258 21.8311 12.926 25.5264 12.926C29.8036 12.926 33.0357 14.5705 35.1905 17.8559L37.2698 21.346L39.3527 17.8559C41.5074 14.5705 44.7395 12.926 49.0095 12.926C52.7013 12.926 55.6784 14.2258 57.9553 16.7572C60.1531 19.2922 61.2508 22.7152 61.2484 27.0263Z" fill="#928374" />
<defs>
<linearGradient id="paint0_linear_549_34" x1="37.0692" y1="0" x2="37.0692" y2="79" gradientUnits="userSpaceOnUse">
<stop stop-color="#6364FF"/>
<stop offset="1" stop-color="#563ACC"/>
</linearGradient>
</defs>
</svg>
</a> </li>
<li> <a href="https://bsky.app/profile/pgaultier.bsky.social">
<svg fill="currentColor" viewBox="0 0 64 57" width="32" style="width: 32px; height: 28.5px;"><path d="M13.873 3.805C21.21 9.332 29.103 20.537 32 26.55v15.882c0-.338-.13.044-.41.867-1.512 4.456-7.418 21.847-20.923 7.944-7.111-7.32-3.819-14.64 9.125-16.85-7.405 1.264-15.73-.825-18.014-9.015C1.12 23.022 0 8.51 0 6.55 0-3.268 8.579-.182 13.873 3.805ZM50.127 3.805C42.79 9.332 34.897 20.537 32 26.55v15.882c0-.338.13.044.41.867 1.512 4.456 7.418 21.847 20.923 7.944 7.111-7.32 3.819-14.64-9.125-16.85 7.405 1.264 15.73-.825 18.014-9.015C62.88 23.022 64 8.51 64 6.55c0-9.818-8.578-6.732-13.873-2.745Z"/></svg>
</a> </li>
</ul>
</div>
<div id="search-matches" hidden>
</div>
<div id="pseudo-body">
<div class="article-prelude">
<p><a href="/blog"> ⏴ Back to all articles</a></p>
<p class="publication-date">Published on 2025-11-21. Last modified on 2025-12-15.</p>
</div>
<div class="article-title">
<h1>A million ways to die from a data race in Go</h1>
<div class="tags"> <a href="/blog/articles-by-tag.html#go" class="tag">Go</a> <a href="/blog/articles-by-tag.html#concurrency" class="tag">Concurrency</a> </div>
</div>
<details class="toc"><summary>Table of contents</summary>
<ul>
<li>
<a href="#accidental-capture-in-a-closure-of-an-outer-variable">Accidental capture in a closure of an outer variable</a>
<ul>
<li>
<a href="#the-fix">The fix</a>
</li>
<li>
<a href="#learnings">Learnings</a>
</li>
</ul>
</li>
<li>
<a href="#concurrent-use-of-http-client">Concurrent use of http.Client</a>
<ul>
<li>
<a href="#the-fix-1">The fix</a>
</li>
<li>
<a href="#learnings-1">Learnings</a>
</li>
</ul>
</li>
<li>
<a href="#improper-lifetime-of-a-mutex">Improper lifetime of a mutex</a>
<ul>
<li>
<a href="#the-fix-2">The fix</a>
</li>
<li>
<a href="#learnings-2">Learnings</a>
</li>
</ul>
</li>
<li>
<a href="#concurrent-reads-and-writes-to-standard-library-containers">Concurrent reads and writes to standard library containers</a>
<ul>
<li>
<a href="#the-fix-3">The fix</a>
</li>
<li>
<a href="#learnings-3">Learnings</a>
</li>
</ul>
</li>
<li>
<a href="#conclusion">Conclusion</a>
</li>
<li>
<a href="#ideas-to-improve-the-status-quo">Ideas to improve the status quo</a>
</li>
</ul>
</details>
<p><em>Comments: <a href="https://www.reddit.com/r/golang/comments/1p5avp3/a_million_ways_to_die_from_a_data_race_in_go/">r/golang</a>, <a href="https://news.ycombinator.com/item?id=46015095">HN</a>.</em></p>
<p>I have been writing production applications in Go for a few years now. I like some aspects of Go. One aspect I do not like is how easy it is to create data races in Go.</p>
<p>Go is often touted for its ease to write highly concurrent programs. However, it is also mind-boggling how many ways Go happily gives us developers to shoot ourselves in the foot.</p>
<p>Over the years I have encountered and fixed many interesting kinds of data races in Go. If that interests you, I have written about Go concurrency in the past and about some existing footguns, without them being necessarily 'Go data races':</p>
<ul>
<li><a href="/blog/what_should_your_mutexes_be_named.html">What should your mutexes be named?</a></li>
<li><a href="/blog/a_subtle_data_race_in_go.html">A subtle data race in Go</a></li>
<li><a href="/blog/subtle_bug_with_go_errgroup.html">A subtle bug with Go's errgroup</a></li>
<li><a href="/blog/how_to_reproduce_and_fix_an_io_data_race_with_dtrace.html">How to reproduce and fix an I/O data race with Go and DTrace</a></li>
</ul>
<p>So what is a 'Go data race'? Quite simply, it is Go code that does not conform to the <a href="https://go.dev/ref/mem">Go memory model</a>. Importantly, Go defines in its memory model what a Go compiler MUST do and MAY do when faced with a non-conforming program exhibiting data races. Not everything is allowed, quite the contrary in fact. Data races in Go are not benign either: their effects can range from 'no symptoms' to 'arbitrary memory corruption'.</p>
<p>Quoting the Go memory model:</p>
<blockquote>
<p>This means that races on multiword data structures can lead to inconsistent values not corresponding to a single write. When the values depend on the consistency of internal (pointer, length) or (pointer, type) pairs, as can be the case for interface values, maps, slices, and strings in most Go implementations, such races can in turn lead to arbitrary memory corruption.</p>
</blockquote>
<p>With this out of the way, let's take a tour of real data races in Go code that I have encountered and fixed. At the end I will emit some recommendations to (try to) avoid them.</p>
<p>I also recommend reading the paper <a href="https://arxiv.org/pdf/2204.00764">A Study of Real-World Data Races in Golang</a>. This article humbly hopes to be a spiritual companion to it. Some items here are also present in this paper, and some are new.</p>
<p>In the code I will often use <code>errgroup.WaitGroup</code> or <code>sync.WaitGroup</code> because they act as a fork-join pattern, shortening the code. The exact same can be done with 'raw' Go channels and goroutines. This also serves to show that using higher-level concepts does not magically protect against all data races.</p>
<h2 id="accidental-capture-in-a-closure-of-an-outer-variable">
<a class="title" href="#accidental-capture-in-a-closure-of-an-outer-variable">Accidental capture in a closure of an outer variable</a>
<a class="hash-anchor" href="#accidental-capture-in-a-closure-of-an-outer-variable" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h2>
<p>This one is very common in Go and also very easy to fall into. Here is a simplified reproducer:</p>
<pre>
<div class="code-header">
<span>Go</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-go"><span class="line-number"></span><span class="code-hl">package main</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">import (</span>
<span class="line-number"></span><span class="code-hl"> "context"</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> "golang.org/x/sync/errgroup"</span>
<span class="line-number"></span><span class="code-hl">)</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func Foo() error { return nil }</span>
<span class="line-number"></span><span class="code-hl">func Bar() error { return nil }</span>
<span class="line-number"></span><span class="code-hl">func Baz() error { return nil }</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func Run(ctx context.Context) error {</span>
<span class="line-number"></span><span class="code-hl"> err := Foo()</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> wg, ctx := errgroup.WithContext(ctx)</span>
<span class="line-number"></span><span class="code-hl"> wg.Go(func() error {</span>
<span class="line-number"></span><span class="code-hl"> err = Baz()</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> return nil</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"> wg.Go(func() error {</span>
<span class="line-number"></span><span class="code-hl"> err = Bar()</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> return nil</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> return wg.Wait()</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func main() {</span>
<span class="line-number"></span><span class="code-hl"> println(Run(context.Background()))</span>
<span class="line-number"></span><span class="code-hl">}</span>
</code></pre>
<p>The issue might not be immediately visible.</p>
<p>The problem is that the <code>err</code> outer variable is implicitly captured by the closures running each in a separate goroutine. They then mutate <code>err</code> concurrently. What they meant to do is instead use a variable local to the closure and return that instead. There is conceptually no need to share any data; this is purely accidental.</p>
<h3 id="the-fix">
<a class="title" href="#the-fix">The fix</a>
<a class="hash-anchor" href="#the-fix" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h3>
<p>The fix is simple, I'll show two variants in the same diff: define a local variable, or use a named return value.</p>
<pre>
<div class="code-header">
<span>Diff</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-diff"><span class="line-number"></span><span class="code-hl">diff --git a/cmd-sg/main.go b/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">index 7eabdbc..4349157 100644</span>
<span class="line-number"></span><span class="code-hl">--- a/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">+++ b/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">@@ -18,14 +18,14 @@ func Run(ctx context.Context) error {</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> wg, ctx := errgroup.WithContext(ctx)</span>
<span class="line-number"></span><span class="code-hl"> wg.Go(func() error {</span>
<span class="line-number"></span><span class="code-hl">- err = Baz()</span>
<span class="line-number"></span><span class="code-hl">+ err := Baz()</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> return nil</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl">- wg.Go(func() error {</span>
<span class="line-number"></span><span class="code-hl">+ wg.Go(func() (err error) {</span>
<span class="line-number"></span><span class="code-hl"> err = Bar()</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
</code></pre>
<h3 id="learnings">
<a class="title" href="#learnings">Learnings</a>
<a class="hash-anchor" href="#learnings" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h3>
<p>It is unfortunate that a one character difference is all we need to fall into this trap. I feel for the original developer who wrote this code without realizing the implicit capture. As mentioned in a <a href="/blog/a_subtle_data_race_in_go.html">previous article</a> where this silent behavior bit me, we can use the build flag <code>-gcflags='-d closure=1'</code> to make the Go compiler print which variables are being captured by the closure:</p>
<pre>
<div class="code-header">
<span>Shell</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-shell"><span class="line-number"></span><span class="code-hl">$ go build -gcflags='-d closure=1' </span>
<span class="line-number"></span><span class="code-hl">./main.go:20:8: heap closure, captured vars = [err]</span>
<span class="line-number"></span><span class="code-hl">./main.go:28:8: heap closure, captured vars = [err]</span>
</code></pre>
<p>But this is not realistic to do that in a big codebase and inspect each closure. It's useful if you know that a given closure might suffer from this problem.</p>
<h2 id="concurrent-use-of-http-client">
<a class="title" href="#concurrent-use-of-http-client">Concurrent use of http.Client</a>
<a class="hash-anchor" href="#concurrent-use-of-http-client" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h2>
<p>The Go docs state about <code>http.Client</code>:</p>
<blockquote>
<p>[...] Clients should be reused instead of created as needed. Clients are safe for concurrent use by multiple goroutines.</p>
</blockquote>
<p>So imagine my surprise when the Go race detector flagged a race tied to <code>http.Client</code>. The code looked like this:</p>
<pre>
<div class="code-header">
<span>Go</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-go"><span class="line-number"></span><span class="code-hl">package main</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">import (</span>
<span class="line-number"></span><span class="code-hl"> "context"</span>
<span class="line-number"></span><span class="code-hl"> "net/http"</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> "golang.org/x/sync/errgroup"</span>
<span class="line-number"></span><span class="code-hl">)</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func Run(ctx context.Context) error {</span>
<span class="line-number"></span><span class="code-hl"> client := http.Client{}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> wg, ctx := errgroup.WithContext(ctx)</span>
<span class="line-number"></span><span class="code-hl"> wg.Go(func() error {</span>
<span class="line-number"></span><span class="code-hl"> client.CheckRedirect = func(req *http.Request, via []*http.Request) error {</span>
<span class="line-number"></span><span class="code-hl"> if req.Host == "google.com" {</span>
<span class="line-number"></span><span class="code-hl"> return nil</span>
<span class="line-number"></span><span class="code-hl"> } else {</span>
<span class="line-number"></span><span class="code-hl"> return http.ErrUseLastResponse</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> _, err := client.Get("http://google.com")</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"> wg.Go(func() error {</span>
<span class="line-number"></span><span class="code-hl"> client.CheckRedirect = nil</span>
<span class="line-number"></span><span class="code-hl"> _, err := client.Get("http://amazon.com")</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> return wg.Wait()</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func main() {</span>
<span class="line-number"></span><span class="code-hl"> println(Run(context.Background()))</span>
<span class="line-number"></span><span class="code-hl">}</span>
</code></pre>
<p>The program makes two concurrent HTTP requests to two different URLs. For the first one, the code restricts redirects (I invented the exact logic for that, no need to look too much into it, the real code has complex logic here). For the second one, no redirect checks are performed, by setting <code>CheckRedirect</code> to nil. This code is idiomatic and follows the recommendations from the documentation:</p>
<blockquote>
<p>CheckRedirect specifies the policy for handling redirects. If CheckRedirect is not nil, the client calls it before following an HTTP redirect.
If CheckRedirect is nil, the Client uses its default policy [...].</p>
</blockquote>
<p>The problem is: the <code>CheckRedirect</code> field is modified concurrently without any synchronization which is a data race.</p>
<p>This code also suffers from an I/O race: depending on the network speed and response time for both URLs, the redirects might or might be checked, since the callback might get overwritten from the other goroutine, right when the HTTP client would call it.</p>
<p>Alternatively, the <code>http.Client</code> could end up calling a <code>nil</code> callback if the callback was set when the <code>http.Client</code> checked whether it was nil or not, but before <code>http.Client</code> had the chance to call it, the other goroutine set it to <code>nil</code>. Boom, <code>nil</code> dereference.</p>
<h3 id="the-fix-1">
<a class="title" href="#the-fix-1">The fix</a>
<a class="hash-anchor" href="#the-fix-1" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h3>
<p>Here, the simplest fix is to use two different HTTP clients:</p>
<pre>
<div class="code-header">
<span>Diff</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-diff"><span class="line-number"></span><span class="code-hl">diff --git a/cmd-sg/main.go b/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">index 351ecc0..8abee1c 100644</span>
<span class="line-number"></span><span class="code-hl">--- a/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">+++ b/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">@@ -8,10 +8,10 @@ import (</span>
<span class="line-number"></span><span class="code-hl"> )</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> func Run(ctx context.Context) error {</span>
<span class="line-number"></span><span class="code-hl">- client := http.Client{}</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> wg, ctx := errgroup.WithContext(ctx)</span>
<span class="line-number"></span><span class="code-hl"> wg.Go(func() error {</span>
<span class="line-number"></span><span class="code-hl">+ client := http.Client{}</span>
<span class="line-number"></span><span class="code-hl"> client.CheckRedirect = func(req *http.Request, via []*http.Request) error {</span>
<span class="line-number"></span><span class="code-hl"> if req.Host == "google.com" {</span>
<span class="line-number"></span><span class="code-hl"> return nil</span>
<span class="line-number"></span><span class="code-hl">@@ -23,6 +23,7 @@ func Run(ctx context.Context) error {</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"> wg.Go(func() error {</span>
<span class="line-number"></span><span class="code-hl">+ client := http.Client{}</span>
<span class="line-number"></span><span class="code-hl"> client.CheckRedirect = nil</span>
<span class="line-number"></span><span class="code-hl"> _, err := client.Get("http://amazon.com")</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
</code></pre>
<h3 id="learnings-1">
<a class="title" href="#learnings-1">Learnings</a>
<a class="hash-anchor" href="#learnings-1" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h3>
<p>This may affect performance negatively since some resources will not be shared anymore.</p>
<p>Additionally, in some situations, this is not so easy because <code>http.Client</code> does not offer a <code>Clone()</code> method (a recurring issue in Go as we'll see). For example, a Go test may start a <code>httptest.Server</code> and then call <code>.Client()</code> on this server to obtain a preconfigured HTTP client for this server. Then, there is no easy way to duplicate this client to use it from two different tests running in parallel.</p>
<p>Again here, I would not blame the original developer. In my view, the docs for <code>http.Client</code> are misleading and should mention that not every single operation is concurrency safe. Perhaps with the wording: 'once a http.Client is constructed, performing a HTTP request is concurrency safe, provided that the http.Client fields are not modified concurrently'. Which is less catchy than 'Clients are safe for concurrent use', period.</p>
<h2 id="improper-lifetime-of-a-mutex">
<a class="title" href="#improper-lifetime-of-a-mutex">Improper lifetime of a mutex</a>
<a class="hash-anchor" href="#improper-lifetime-of-a-mutex" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h2>
<p>The next data race is one that baffled me for a bit, because the code was using a mutex properly and I could not fathom why a race would be possible.</p>
<p>Here's a minimal reproducer:</p>
<pre>
<div class="code-header">
<span>Go</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-go"><span class="line-number"></span><span class="code-hl">package main</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">import (</span>
<span class="line-number"></span><span class="code-hl"> "encoding/json"</span>
<span class="line-number"></span><span class="code-hl"> "net/http"</span>
<span class="line-number"></span><span class="code-hl"> "sync"</span>
<span class="line-number"></span><span class="code-hl">)</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">type Plans map[string]int</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">type PricingInfo struct {</span>
<span class="line-number"></span><span class="code-hl"> plans Plans</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">var pricingInfo = PricingInfo{plans: Plans{"cheap plan": 1, "expensive plan": 5}}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">type PricingService struct {</span>
<span class="line-number"></span><span class="code-hl"> info PricingInfo</span>
<span class="line-number"></span><span class="code-hl"> infoMtx sync.Mutex</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func NewPricingService() *PricingService {</span>
<span class="line-number"></span><span class="code-hl"> return &PricingService{info: pricingInfo, infoMtx: sync.Mutex{}}</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func AddPricing(w http.ResponseWriter, r *http.Request) {</span>
<span class="line-number"></span><span class="code-hl"> pricingService := NewPricingService()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> pricingService.infoMtx.Lock()</span>
<span class="line-number"></span><span class="code-hl"> defer pricingService.infoMtx.Unlock()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> pricingService.info.plans["middle plan"] = 3</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> encoder := json.NewEncoder(w)</span>
<span class="line-number"></span><span class="code-hl"> encoder.Encode(pricingService.info)</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func GetPricing(w http.ResponseWriter, r *http.Request) {</span>
<span class="line-number"></span><span class="code-hl"> pricingService := NewPricingService()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> pricingService.infoMtx.Lock()</span>
<span class="line-number"></span><span class="code-hl"> defer pricingService.infoMtx.Unlock()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> encoder := json.NewEncoder(w)</span>
<span class="line-number"></span><span class="code-hl"> encoder.Encode(pricingService.info)</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func main() {</span>
<span class="line-number"></span><span class="code-hl"> http.HandleFunc("POST /add-pricing", AddPricing)</span>
<span class="line-number"></span><span class="code-hl"> http.HandleFunc("GET /pricing", GetPricing)</span>
<span class="line-number"></span><span class="code-hl"> http.ListenAndServe(":12345", nil)</span>
<span class="line-number"></span><span class="code-hl">}</span>
</code></pre>
<p>A global mutable map of pricing information is guarded by a mutex. One HTTP endpoint reads the map, another adds an item to it. Pretty simple I would say. The locking is done correctly.</p>
<p>Yet the map suffers from a data race. Here is a reproducer:</p>
<pre>
<div class="code-header">
<span>Go</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-go"><span class="line-number"></span><span class="code-hl">package main</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">import (</span>
<span class="line-number"></span><span class="code-hl"> "net/http"</span>
<span class="line-number"></span><span class="code-hl"> "net/http/httptest"</span>
<span class="line-number"></span><span class="code-hl"> "testing"</span>
<span class="line-number"></span><span class="code-hl">)</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func TestMain(t *testing.T) {</span>
<span class="line-number"></span><span class="code-hl"> mux := http.NewServeMux()</span>
<span class="line-number"></span><span class="code-hl"> mux.HandleFunc("POST /add-pricing", AddPricing)</span>
<span class="line-number"></span><span class="code-hl"> mux.HandleFunc("GET /pricing", GetPricing)</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> server := httptest.NewServer(mux)</span>
<span class="line-number"></span><span class="code-hl"> t.Cleanup(server.Close)</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> t.Run("get pricing", func(t *testing.T) {</span>
<span class="line-number"></span><span class="code-hl"> t.Parallel()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> _, err := server.Client().Get(server.URL + "/pricing")</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> panic(err)</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> for range 5 {</span>
<span class="line-number"></span><span class="code-hl"> t.Run("add pricing", func(t *testing.T) {</span>
<span class="line-number"></span><span class="code-hl"> t.Parallel()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> _, err := server.Client().Post(server.URL+"/add-pricing", "application/json", nil)</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> panic(err)</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl">}</span>
</code></pre>
<p>The reason why is because the data, and the mutex guarding it, do not have the same 'lifetime'. The <code>pricingInfo</code> map is global and exists from the start of the program to the end. But the mutex <code>infoMtx</code> exists only for the duration of the HTTP handler (and thus HTTP request). We effectively have 1 map and N mutexes, none of them shared between HTTP handlers. So HTTP handlers cannot synchronize access to the map.</p>
<p>The intent of the code was (I think) to do a deep clone of the pricing information at the beginning of the HTTP handler in <code>NewPricingService</code>. Unfortunately, Go does a shallow copy of structures and thus each <code>PricingService</code> instance ends up sharing the same underlying <code>plans</code> map, which is this global map. It could be that for a long time, it worked because the <code>PricingInfo</code> struct did not yet contain the map (in the real code it contains a lot of <code>int</code>s and <code>string</code>s which are value types and will be copied correctly by a shallow copy), and the map was only added later.</p>
<p>This data race is akin to copying a mutex by value when passing it to a function, which then locks it. This does no synchronization at all since a copy of the mutex is being locked - no mutex is shared between concurrent units.</p>
<h3 id="the-fix-2">
<a class="title" href="#the-fix-2">The fix</a>
<a class="hash-anchor" href="#the-fix-2" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h3>
<p>In any event, the fix is to align the lifetime of the data and the mutex:</p>
<ul>
<li>We can keep the map global and make the mutex also global, so that it is shared by every HTTP handler, thus we have 1 map and 1 mutex; or</li>
<li>We make the map scoped to the HTTP handler by implementing a deep clone function, thus we have N maps and N mutexes</li>
</ul>
<p>I went with the second approach in the real code because it seemed to be the original intent:</p>
<pre>
<div class="code-header">
<span>Diff</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-diff"><span class="line-number"></span><span class="code-hl">diff --git a/cmd-sg/main.go b/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">index fb59f5c..c7a7a94 100644</span>
<span class="line-number"></span><span class="code-hl">--- a/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">+++ b/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">@@ -2,6 +2,7 @@ package main</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> import (</span>
<span class="line-number"></span><span class="code-hl"> "encoding/json"</span>
<span class="line-number"></span><span class="code-hl">+ "maps"</span>
<span class="line-number"></span><span class="code-hl"> "net/http"</span>
<span class="line-number"></span><span class="code-hl"> "sync"</span>
<span class="line-number"></span><span class="code-hl"> )</span>
<span class="line-number"></span><span class="code-hl">@@ -19,8 +20,15 @@ type PricingService struct {</span>
<span class="line-number"></span><span class="code-hl"> infoMtx sync.Mutex</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl">+func ClonePricing(pricingInfo PricingInfo) PricingInfo {</span>
<span class="line-number"></span><span class="code-hl">+ cloned := PricingInfo{plans: make(Plans, len(pricingInfo.plans))}</span>
<span class="line-number"></span><span class="code-hl">+ maps.Copy(cloned.plans, pricingInfo.plans)</span>
<span class="line-number"></span><span class="code-hl">+</span>
<span class="line-number"></span><span class="code-hl">+ return cloned</span>
<span class="line-number"></span><span class="code-hl">+}</span>
<span class="line-number"></span><span class="code-hl">+</span>
<span class="line-number"></span><span class="code-hl"> func NewPricingService() *PricingService {</span>
<span class="line-number"></span><span class="code-hl">- return &PricingService{info: pricingInfo, infoMtx: sync.Mutex{}}</span>
<span class="line-number"></span><span class="code-hl">+ return &PricingService{info: ClonePricing(pricingInfo), infoMtx: sync.Mutex{}}</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> func AddPricing(w http.ResponseWriter, r *http.Request) {</span>
</code></pre>
<h3 id="learnings-2">
<a class="title" href="#learnings-2">Learnings</a>
<a class="hash-anchor" href="#learnings-2" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h3>
<p>It's annoying to have to implement this manually and especially to have to check every single nested field to determine if it's a value type or a reference type (the former will behave correctly with a shallow copy, the latter needs a custom deep copy implementation). I miss the <code>derive(Clone)</code> annotation in Rust. This is something that the compiler can (and should) do better than me.</p>
<p>Furthermore, as mentioned in the previous section, some types from the standard library or third-party libraries do not implement a deep <code>Clone()</code> function and contain private fields which prevent us from implementing that ourselves.</p>
<p>I think Rust's API for a mutex is better because a Rust mutex wraps the data it protects and thus it is harder to have uncorrelated lifetimes for the data and the mutex.</p>
<p>Go's mutex API likely could not have been implemented this way since it would have required generics which did not exist at the time. But as of today: I think it could.</p>
<p>Nonetheless, the Go compiler has no way to detect accidental shallow copying, whereas Rust's compiler has the concepts of <code>Copy</code> and <code>Clone</code> - so that issue remains in Go, and is not a simple API mistake in the standard library we can fix.</p>
<h2 id="concurrent-reads-and-writes-to-standard-library-containers">
<a class="title" href="#concurrent-reads-and-writes-to-standard-library-containers">Concurrent reads and writes to standard library containers</a>
<a class="hash-anchor" href="#concurrent-reads-and-writes-to-standard-library-containers" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h2>
<p>I encountered many cases of concurrently modifying a <code>map</code>, <code>slice</code>, etc without any synchronization. That's your run of the mill data race and they are typically fixed by 'slapping a mutex on it' or using a concurrency safe data structure such as <code>sync.Map</code>.</p>
<p>I will thus share here a more interesting one where it is not as straightforward.</p>
<p>This time, the code is convoluted but what it does is relatively simple:</p>
<ol>
<li>Spawn a docker container and capture its standard output in a byte buffer</li>
<li>Concurrently (in a different goroutine), read this output and find a given token.</li>
<li>Once the token is found, the context is canceled and thus the container is automatically stopped.</li>
<li>The token is returned</li>
</ol>
<pre>
<div class="code-header">
<span>Go</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-go"><span class="line-number"></span><span class="code-hl">package main</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">import (</span>
<span class="line-number"></span><span class="code-hl"> "bytes"</span>
<span class="line-number"></span><span class="code-hl"> "context"</span>
<span class="line-number"></span><span class="code-hl"> "io"</span>
<span class="line-number"></span><span class="code-hl"> "strings"</span>
<span class="line-number"></span><span class="code-hl"> "time"</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> "github.com/ory/dockertest/v3"</span>
<span class="line-number"></span><span class="code-hl"> "github.com/ory/dockertest/v3/docker"</span>
<span class="line-number"></span><span class="code-hl"> "golang.org/x/sync/errgroup"</span>
<span class="line-number"></span><span class="code-hl">)</span>
<span class="line-number"></span><span class="code-hl">func GetSigningSecretFromStripeContainer() string {</span>
<span class="line-number"></span><span class="code-hl"> dp, err := dockertest.NewPool("")</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> panic(err)</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> forwarder, err := dp.RunWithOptions(&dockertest.RunOptions{</span>
<span class="line-number"></span><span class="code-hl"> Repository: "stripe/stripe-cli",</span>
<span class="line-number"></span><span class="code-hl"> Tag: "v1.19.1",</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> panic(err)</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> output := &bytes.Buffer{}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> ctx, cancel := context.WithCancel(context.Background())</span>
<span class="line-number"></span><span class="code-hl"> defer cancel()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> var signingSecret string</span>
<span class="line-number"></span><span class="code-hl"> eg := errgroup.Group{}</span>
<span class="line-number"></span><span class="code-hl"> eg.Go(func() error {</span>
<span class="line-number"></span><span class="code-hl"> defer cancel()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> for {</span>
<span class="line-number"></span><span class="code-hl"> ln, err := output.ReadString('\n')</span>
<span class="line-number"></span><span class="code-hl"> if err == io.EOF {</span>
<span class="line-number"></span><span class="code-hl"> <-time.After(100 * time.Millisecond)</span>
<span class="line-number"></span><span class="code-hl"> continue</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl"> return err</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> if strings.Contains(ln, "Ready!") {</span>
<span class="line-number"></span><span class="code-hl"> ln = ln[strings.Index(ln, "whsec_"):]</span>
<span class="line-number"></span><span class="code-hl"> signingSecret = ln[:strings.Index(ln, " ")]</span>
<span class="line-number"></span><span class="code-hl"> return nil</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> dp.Client.Logs(docker.LogsOptions{</span>
<span class="line-number"></span><span class="code-hl"> Context: ctx,</span>
<span class="line-number"></span><span class="code-hl"> Stderr: true,</span>
<span class="line-number"></span><span class="code-hl"> Follow: true,</span>
<span class="line-number"></span><span class="code-hl"> RawTerminal: true,</span>
<span class="line-number"></span><span class="code-hl"> Container: forwarder.Container.ID,</span>
<span class="line-number"></span><span class="code-hl"> OutputStream: output,</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> eg.Wait()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> return signingSecret</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func main() {</span>
<span class="line-number"></span><span class="code-hl"> println(GetSigningSecretFromStripeContainer())</span>
<span class="line-number"></span><span class="code-hl">}</span>
</code></pre>
<p>So, the issue may be clear from the description but here it is spelled out: one goroutine writes to a (growing) byte buffer, another one reads from it, and there is no synchronization: that's a clear data race.</p>
<p>What is interesting here is that we have to pass an <code>io.Writer</code> for the <code>OutputStream</code> to the library, and this library will write to the writer we passed. We cannot insert a mutex lock anywhere around the write site, since we do not control the library and there no hooks (e.g. pre/post write callbacks) to do so.</p>
<h3 id="the-fix-3">
<a class="title" href="#the-fix-3">The fix</a>
<a class="hash-anchor" href="#the-fix-3" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h3>
<p>We implement our own writer that does the synchronization with a mutex:</p>
<pre>
<div class="code-header">
<span>Go</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-go"><span class="line-number"></span><span class="code-hl">type SyncWriter struct {</span>
<span class="line-number"></span><span class="code-hl"> Writer io.Writer</span>
<span class="line-number"></span><span class="code-hl"> Mtx *sync.Mutex</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func NewSyncWriter(w io.Writer, mtx *sync.Mutex) io.Writer {</span>
<span class="line-number"></span><span class="code-hl"> return &SyncWriter{Writer: w, Mtx: mtx}</span>
<span class="line-number"></span><span class="code-hl">}</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl">func (w *SyncWriter) Write(p []byte) (n int, err error) {</span>
<span class="line-number"></span><span class="code-hl"> w.Mtx.Lock()</span>
<span class="line-number"></span><span class="code-hl"> defer w.Mtx.Unlock()</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> written, err := w.Writer.Write(p)</span>
<span class="line-number"></span><span class="code-hl"></span>
<span class="line-number"></span><span class="code-hl"> return written, err</span>
<span class="line-number"></span><span class="code-hl">}</span>
</code></pre>
<p>We pass it as is to the third-party library, and when we want to read the byte buffer, we lock the mutex first:</p>
<pre>
<div class="code-header">
<span>Diff</span>
<button class="copy-code" type="button"><svg aria-hidden="true" focusable="false" class="octicon octicon-copy" viewBox="0 0 16 16" width="16" height="16" fill="currentColor" display="inline-block" overflow="visible" style="vertical-align: text-bottom;"><path d="M0 6.75C0 5.784.784 5 1.75 5h1.5a.75.75 0 0 1 0 1.5h-1.5a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-1.5a.75.75 0 0 1 1.5 0v1.5A1.75 1.75 0 0 1 9.25 16h-7.5A1.75 1.75 0 0 1 0 14.25Z"></path><path d="M5 1.75C5 .784 5.784 0 6.75 0h7.5C15.216 0 16 .784 16 1.75v7.5A1.75 1.75 0 0 1 14.25 11h-7.5A1.75 1.75 0 0 1 5 9.25Zm1.75-.25a.25.25 0 0 0-.25.25v7.5c0 .138.112.25.25.25h7.5a.25.25 0 0 0 .25-.25v-7.5a.25.25 0 0 0-.25-.25Z"></path></svg></button>
</div>
<code class="language-diff"><span class="line-number"></span><span class="code-hl">diff --git a/cmd-sg/main.go b/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">index 5529d90..42571b9 100644</span>
<span class="line-number"></span><span class="code-hl">--- a/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">+++ b/cmd-sg/main.go</span>
<span class="line-number"></span><span class="code-hl">@@ -5,6 +5,7 @@ import (</span>
<span class="line-number"></span><span class="code-hl"> "context"</span>
<span class="line-number"></span><span class="code-hl"> "io"</span>
<span class="line-number"></span><span class="code-hl"> "strings"</span>
<span class="line-number"></span><span class="code-hl">+ "sync"</span>
<span class="line-number"></span><span class="code-hl"> "time"</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> "github.com/ory/dockertest/v3"</span>
<span class="line-number"></span><span class="code-hl">@@ -12,6 +13,24 @@ import (</span>
<span class="line-number"></span><span class="code-hl"> "golang.org/x/sync/errgroup"</span>
<span class="line-number"></span><span class="code-hl"> )</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl">+type SyncWriter struct {</span>
<span class="line-number"></span><span class="code-hl">+ Writer io.Writer</span>
<span class="line-number"></span><span class="code-hl">+ Mtx *sync.Mutex</span>
<span class="line-number"></span><span class="code-hl">+}</span>
<span class="line-number"></span><span class="code-hl">+</span>
<span class="line-number"></span><span class="code-hl">+func NewSyncWriter(w io.Writer, mtx *sync.Mutex) io.Writer {</span>
<span class="line-number"></span><span class="code-hl">+ return &SyncWriter{Writer: w, Mtx: mtx}</span>
<span class="line-number"></span><span class="code-hl">+}</span>
<span class="line-number"></span><span class="code-hl">+</span>
<span class="line-number"></span><span class="code-hl">+func (w *SyncWriter) Write(p []byte) (n int, err error) {</span>
<span class="line-number"></span><span class="code-hl">+ w.Mtx.Lock()</span>
<span class="line-number"></span><span class="code-hl">+ defer w.Mtx.Unlock()</span>
<span class="line-number"></span><span class="code-hl">+</span>
<span class="line-number"></span><span class="code-hl">+ written, err := w.Writer.Write(p)</span>
<span class="line-number"></span><span class="code-hl">+</span>
<span class="line-number"></span><span class="code-hl">+ return written, err</span>
<span class="line-number"></span><span class="code-hl">+}</span>
<span class="line-number"></span><span class="code-hl">+</span>
<span class="line-number"></span><span class="code-hl"> func GetSigningSecretFromStripeContainer() string {</span>
<span class="line-number"></span><span class="code-hl"> dp, err := dockertest.NewPool("")</span>
<span class="line-number"></span><span class="code-hl"> if err != nil {</span>
<span class="line-number"></span><span class="code-hl">@@ -27,6 +46,8 @@ func GetSigningSecretFromStripeContainer() string {</span>
<span class="line-number"></span><span class="code-hl"> }</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> output := &bytes.Buffer{}</span>
<span class="line-number"></span><span class="code-hl">+ outputMtx := sync.Mutex{}</span>
<span class="line-number"></span><span class="code-hl">+ writer := NewSyncWriter(output, &outputMtx)</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> ctx, cancel := context.WithCancel(context.Background())</span>
<span class="line-number"></span><span class="code-hl"> defer cancel()</span>
<span class="line-number"></span><span class="code-hl">@@ -37,7 +58,9 @@ func GetSigningSecretFromStripeContainer() string {</span>
<span class="line-number"></span><span class="code-hl"> defer cancel()</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> for {</span>
<span class="line-number"></span><span class="code-hl">+ outputMtx.Lock()</span>
<span class="line-number"></span><span class="code-hl"> ln, err := output.ReadString('\n')</span>
<span class="line-number"></span><span class="code-hl">+ outputMtx.Unlock()</span>
<span class="line-number"></span><span class="code-hl"> if err == io.EOF {</span>
<span class="line-number"></span><span class="code-hl"> <-time.After(100 * time.Millisecond)</span>
<span class="line-number"></span><span class="code-hl"> continue</span>
<span class="line-number"></span><span class="code-hl">@@ -59,7 +82,7 @@ func GetSigningSecretFromStripeContainer() string {</span>
<span class="line-number"></span><span class="code-hl"> Follow: true,</span>
<span class="line-number"></span><span class="code-hl"> RawTerminal: true,</span>
<span class="line-number"></span><span class="code-hl"> Container: forwarder.Container.ID,</span>
<span class="line-number"></span><span class="code-hl">- OutputStream: output,</span>
<span class="line-number"></span><span class="code-hl">+ OutputStream: writer,</span>
<span class="line-number"></span><span class="code-hl"> })</span>
<span class="line-number"></span><span class="code-hl"> </span>
<span class="line-number"></span><span class="code-hl"> eg.Wait()</span>
</code></pre>
<p>Note: A <a href="https://news.ycombinator.com/item?id=46050516">comment</a> pointed out that <code>reader, writer := io.Pipe()</code> is tailored exactly for this. Indeed, this is an elegant solution: we just have to also wrap the reader returned by <code>io.Pipe()</code> in a <code>bufio.NewReader()</code> which gives us the <code>ReadString('\n')</code> method. So, this is probably the preferrable solution. As it is often the case in Go, there are two ways to solve this concurrency problem: with a mutex (the custom <code>SyncWriter</code>) or with a channel (which <code>io.Pipe()</code> uses under the hood).</p>
<h3 id="learnings-3">
<a class="title" href="#learnings-3">Learnings</a>
<a class="hash-anchor" href="#learnings-3" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h3>
<p>Most types in the Go standard library (or third-party libraries for that matter) are <em>not</em> concurrency safe and synchronization is typically on you. I still often see questions on the internet about that, so assume it is not until the documentation states otherwise.</p>
<p>It would also be nice if more types have a 'sync' version, e.g. <code>SyncWriter</code>, <code>SyncReader</code>, etc.</p>
<h2 id="conclusion">
<a class="title" href="#conclusion">Conclusion</a>
<a class="hash-anchor" href="#conclusion" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h2>
<p>The Go race detector is great but will not detect all data races. Data races will cause you pain and suffering, be it flaky tests, weird production errors, or in the worst case memory corruption.</p>
<p>Due to how easy it is to spawn goroutines without a care in the world (and also to run tests in parallel), it will happen to you. It's not a question of if, just when, how bad, and how many days/weeks it will cost you to find them and fix them.</p>
<p>If you are not running your test suite with the race detector enabled, you have numerous data races in your code. That's just a fact.</p>
<p>Go the language and the Go linter ecosystem do not have nearly enough answers to this problem. Some language features make it way too easy to trigger data races, for example implicit capture of outer variables in closures.</p>
<p>The best option left to Go developers is to try to reach 100% test coverage of their code and run the tests with the race detector on.</p>
<p>We should be able to do better in 2025. Just like with memory safety, when even expert developers regularly produce data races, it's the fault of the language/tooling/APIs/etc. It is not enough to blame humans and demand they just 'do better'.</p>
<h2 id="ideas-to-improve-the-status-quo">
<a class="title" href="#ideas-to-improve-the-status-quo">Ideas to improve the status quo</a>
<a class="hash-anchor" href="#ideas-to-improve-the-status-quo" aria-hidden="true" onclick="navigator.clipboard.writeText(this.href);"></a>
</h2>
<p>Ideas for the Go language:</p>
<ol>
<li>Add explicit capture lists for closures, just like in C++.</li>
<li>Add a lint to forbid using the implicit capture syntax in closures (a.k.a.: current Go closures). I am fine writing a stand-alone plain function instead, if that means keeping my sanity and removing an entire category of errors. I have also seen implicit captures cause logic bugs and high memory usage in the past.</li>
<li>Support <code>const</code> in more places. If something is constant, there cannot be data races with it.</li>
<li>Generate a <code>Clone()</code> function in the compiler for every type (like Rust's <code>derive(Clone)</code>). Maybe it's opt-in or opt-out, not sure. Or perhaps it's a built-in like <code>make</code>.</li>
<li>Add a <code>freeze()</code> functionality like JavaScript's <code>Object.freeze()</code> to prevent an object from being mutated.</li>
<li>Expand the standard library documentation to have more details concerning the concurrency safety of certain types and APIs.</li>
<li>Expand the Go memory model documentation and add examples. I have read it many times and I am still unsure if concurrent writes to separate fields of a struct is legal or not, for example.</li>
<li>Consider adding better, higher-level APIs for synchronization primitives e.g. <code>Mutex</code> by taking inspiration from other languages. This has been done successfully in the past with <code>WaitGroup</code> compared to using raw goroutines and channels.</li>
</ol>
<p>Ideas for Go programs:</p>
<ol>
<li>Consider never using Go closures, and instead using plain functions that cannot implicit capture outer variables.</li>
<li>Consider using goroutines as little as possible, whatever the API to manage them.</li>
<li>Consider spawning an OS process instead of a goroutine for isolation. No sharing of data means no data race possible.</li>
<li>Deep clone abundantly (just like in Rust). Memory (especially cache) is lightning fast. Your Go program will anyways not be bottlenecked by that, I guarantee it. Memory usage should be monitored, though, but will probably be fine.</li>
<li>Avoid global mutable variables.</li>
<li>Carefully audit resource sharing code: caches, connection pools, OS process pools, HTTP clients, etc. They are likely to contain data races.</li>
<li>Run the tests with the race detector on, all of them, always, from day one. Inspect the test coverage to know which areas may be uncharted areas in terms of concurrency safety.</li>
<li>Study places where a shallow copy may take place, e.g. function arguments passed by value and assignments. Does the type require a deep copy? Each non-trivial type should have documentation stating that.</li>
<li>If a type can be implemented in an immutable fashion, then it's great because there is no data race possible. For example, the <code>string</code> type in Go is immutable.</li>
</ol>
<p><a href="/blog"> ⏴ Back to all articles</a></p>
<div id="donate">
<em>
<p>If you enjoy what you're reading, you want to support me, and can afford it: <a href="https://paypal.me/philigaultier?country.x=DE&locale.x=en_US">Support me</a>. That allows me to write more cool articles!</p>
<p>
This blog is <a href="https://github.com/gaultier/blog">open-source</a>!
If you find a problem, please open a Github issue.
The content of this blog as well as the code snippets are under the <a href="https://en.wikipedia.org/wiki/BSD_licenses#3-clause_license_(%22BSD_License_2.0%22,_%22Revised_BSD_License%22,_%22New_BSD_License%22,_or_%22Modified_BSD_License%22)">BSD-3 License</a> which I also usually use for all my personal projects. It's basically free for every use but you have to mention me as the original author.
</p>
</em>
</div>
</div>
</body>
</html>