authorgravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2021-12-09 21:14:39-07:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2021-12-09 22:10:38-07:00
logfdbb329d1009b4c7b23552e65c0344e3ce52b459
treeba83854a4725344ffb9132589ca577b2f947a439
parent4da83feccb4b0e59afdcce9796b08cc4fc8346ae

Cache: fix data race with is_problematic_timestamp

Previously `recent_problematic_timestamp` was unprotected and accessed potentially with multiple worker threads simultaneously. This commit protects it with atomics and also introduces a flag to prevent multiple timestamp checks from within the same call to hit(). Unfortunately the compiler-rt function __sync_val_compare_and_swap_16 is not yet implemented, so I will have to take a different strategy in a follow-up commit.

1 files changed, 26 insertions(+), 8 deletions(-)

src/Cache.zig+26-8
...@@ -2,6 +2,7 @@ gpa: Allocator,...@@ -2,6 +2,7 @@ gpa: Allocator,
2manifest_dir: fs.Dir,2manifest_dir: fs.Dir,
3hash: HashHelper = .{},3hash: HashHelper = .{},
4recent_problematic_timestamp: i128 = 0,4recent_problematic_timestamp: i128 = 0,
5want_refresh_timestamp: bool = true,
56
6const Cache = @This();7const Cache = @This();
7const std = @import("std");8const std = @import("std");
...@@ -346,6 +347,12 @@ pub const Manifest = struct {...@@ -346,6 +347,12 @@ pub const Manifest = struct {
346 }347 }
347 }348 }
348349
350 // Indicate that we want isProblematicTimestamp to perform a filesystem write in
351 // order to obtain a problematic timestamp for the next call. Calls after that
352 // in this same hit() function call will then use the same timestamp, to avoid
353 // writing multiple times to the filesystem.
354 @atomicStore(bool, &self.cache.want_refresh_timestamp, true, .Monotonic);
355
349 const file_contents = try self.manifest_file.?.reader().readAllAlloc(self.cache.gpa, manifest_file_size_max);356 const file_contents = try self.manifest_file.?.reader().readAllAlloc(self.cache.gpa, manifest_file_size_max);
350 defer self.cache.gpa.free(file_contents);357 defer self.cache.gpa.free(file_contents);
351358
...@@ -749,18 +756,29 @@ fn hashFile(file: fs.File, bin_digest: *[Hasher.mac_length]u8) !void {...@@ -749,18 +756,29 @@ fn hashFile(file: fs.File, bin_digest: *[Hasher.mac_length]u8) !void {
749fn isProblematicTimestamp(cache: *Cache, file_time: i128) bool {756fn isProblematicTimestamp(cache: *Cache, file_time: i128) bool {
750 // If the file_time is prior to the most recent problematic timestamp757 // If the file_time is prior to the most recent problematic timestamp
751 // then we don't need to access the filesystem.758 // then we don't need to access the filesystem.
752 if (file_time < cache.recent_problematic_timestamp)759 var ts = @atomicLoad(i128, &cache.recent_problematic_timestamp, .Monotonic);
760 if (file_time < ts)
753 return false;761 return false;
754762
755 var file = cache.manifest_dir.createFile("timestamp", .{763 if (@atomicRmw(bool, &cache.want_refresh_timestamp, .Xchg, false, .Monotonic)) {
756 .read = true,764 var file = cache.manifest_dir.createFile("timestamp", .{
757 .truncate = true,765 .read = true,
758 }) catch return true;766 .truncate = true,
759 defer file.close();767 }) catch return true;
768 defer file.close();
760769
761 cache.recent_problematic_timestamp = (file.stat() catch return true).mtime;770 const new_ts = (file.stat() catch return true).mtime;
771 ts = if (@cmpxchgWeak(
772 i128,
773 &cache.recent_problematic_timestamp,
774 ts,
775 new_ts,
776 .Monotonic,
777 .Monotonic,
778 )) |race_ts| race_ts else new_ts;
779 }
762780
763 return file_time >= cache.recent_problematic_timestamp;781 return file_time >= ts;
764}782}
765783
766// Create/Write a file, close it, then grab its stat.mtime timestamp.784// Create/Write a file, close it, then grab its stat.mtime timestamp.