Skip to content

Commit a3e2ee0

Browse files
authored
Fix another LockViolation case on Windows (#14162)
- Add an assert that an exclusive lock is help to writeManifest - Only call writeManifest in updateCObject if an exclusive lock is held - cache: fixup test to verify hits don't take an exclusive lock, instead of writing the manifest
1 parent 9ed4a93 commit a3e2ee0

File tree

2 files changed

+23
-17
lines changed

2 files changed

+23
-17
lines changed

src/Cache.zig

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -830,6 +830,8 @@ pub const Manifest = struct {
830830
/// If `want_shared_lock` is true, this function automatically downgrades the
831831
/// lock from exclusive to shared.
832832
pub fn writeManifest(self: *Manifest) !void {
833+
assert(self.have_exclusive_lock);
834+
833835
const manifest_file = self.manifest_file.?;
834836
if (self.manifest_dirty) {
835837
self.manifest_dirty = false;
@@ -1033,7 +1035,7 @@ test "cache file and then recall it" {
10331035
try testing.expect(try ch.hit());
10341036
digest2 = ch.final();
10351037

1036-
try ch.writeManifest();
1038+
try testing.expectEqual(false, ch.have_exclusive_lock);
10371039
}
10381040

10391041
try testing.expectEqual(digest1, digest2);
@@ -1161,7 +1163,7 @@ test "no file inputs" {
11611163

11621164
try testing.expect(try man.hit());
11631165
digest2 = man.final();
1164-
try man.writeManifest();
1166+
try testing.expectEqual(false, man.have_exclusive_lock);
11651167
}
11661168

11671169
try testing.expectEqual(digest1, digest2);
@@ -1224,7 +1226,7 @@ test "Manifest with files added after initial hash work" {
12241226
try testing.expect(try ch.hit());
12251227
digest2 = ch.final();
12261228

1227-
try ch.writeManifest();
1229+
try testing.expectEqual(false, ch.have_exclusive_lock);
12281230
}
12291231
try testing.expect(mem.eql(u8, &digest1, &digest2));
12301232

src/Compilation.zig

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3681,13 +3681,15 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {
36813681
break :digest digest;
36823682
} else man.final();
36833683

3684-
// Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
3685-
// possible we had a hit and the manifest is dirty, for example if the file mtime changed but
3686-
// the contents were the same, we hit the cache but the manifest is dirty and we need to update
3687-
// it to prevent doing a full file content comparison the next time around.
3688-
man.writeManifest() catch |err| {
3689-
log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});
3690-
};
3684+
if (man.have_exclusive_lock) {
3685+
// Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
3686+
// possible we had a hit and the manifest is dirty, for example if the file mtime changed but
3687+
// the contents were the same, we hit the cache but the manifest is dirty and we need to update
3688+
// it to prevent doing a full file content comparison the next time around.
3689+
man.writeManifest() catch |err| {
3690+
log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});
3691+
};
3692+
}
36913693

36923694
const out_zig_path = try comp.local_cache_directory.join(comp.gpa, &[_][]const u8{
36933695
"o", &digest, cimport_zig_basename,
@@ -4086,13 +4088,15 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P
40864088
break :blk digest;
40874089
};
40884090

4089-
// Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
4090-
// possible we had a hit and the manifest is dirty, for example if the file mtime changed but
4091-
// the contents were the same, we hit the cache but the manifest is dirty and we need to update
4092-
// it to prevent doing a full file content comparison the next time around.
4093-
man.writeManifest() catch |err| {
4094-
log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });
4095-
};
4091+
if (man.have_exclusive_lock) {
4092+
// Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
4093+
// possible we had a hit and the manifest is dirty, for example if the file mtime changed but
4094+
// the contents were the same, we hit the cache but the manifest is dirty and we need to update
4095+
// it to prevent doing a full file content comparison the next time around.
4096+
man.writeManifest() catch |err| {
4097+
log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });
4098+
};
4099+
}
40964100

40974101
const o_basename = try std.fmt.allocPrint(arena, "{s}{s}", .{ o_basename_noext, o_ext });
40984102

0 commit comments

Comments
 (0)