hare

The Hare programming language
git clone https://git.torresjrjr.com/hare.git
Log | Files | Refs | README | LICENSE

commit 2da3ed216a288efe48979a96acbafede45866ebd
parent 80e901354972f70791d7e889d347c2ab7a164ded
Author: Drew DeVault <sir@cmpwn.com>
Date:   Sun, 27 Jun 2021 10:54:54 -0400

hare::module: use lock-free atomic manifest writes

This fixes a race condition in parallel builds by writing the manifest
to a temporary path and then moving it into its final position.

Signed-off-by: Drew DeVault <sir@cmpwn.com>

Diffstat:
Mhare/module/manifest.ha | 53+++++++----------------------------------------------
Mtemp/+linux.ha | 12+++++++-----
2 files changed, 14 insertions(+), 51 deletions(-)

diff --git a/hare/module/manifest.ha b/hare/module/manifest.ha @@ -13,6 +13,7 @@ use path; use strconv; use strings; use time; +use temp; // The manifest file format is a series of line-oriented records. Lines starting // with # are ignored. @@ -51,11 +52,6 @@ export fn manifest_load(ctx: *context, ident: ast::ident) (manifest | error) = { let mpath = path::join(cachedir, "manifest"); defer free(mpath); - // TODO: We can probably eliminate these locks by using atomic writes - // instead - let l = lock(ctx.fs, cachedir)?; - defer unlock(ctx.fs, cachedir, l); - let file = match (fs::open(ctx.fs, mpath, fs::flags::RDONLY)) { errors::noentry => return manifest, err: fs::error => return err, @@ -237,11 +233,11 @@ export fn manifest_write(ctx: *context, manifest: *manifest) (void | error) = { let mpath = path::join(cachedir, "manifest"); defer free(mpath); - let l = lock(ctx.fs, cachedir)?; - defer unlock(ctx.fs, cachedir, l); - - let fd = fs::create(ctx.fs, mpath, 0o644)?; - defer io::close(fd); + let fd = temp::named(ctx.fs, cachedir, io::mode::WRITE, 0o644)?; + defer { + fs::remove(ctx.fs, fd.name): void; + io::close(fd); + }; let ident = unparse::identstr(manifest.ident); defer free(ident); @@ -277,43 +273,8 @@ export fn manifest_write(ctx: *context, manifest: *manifest) (void | error) = { fmt::fprintln(fd)?; }; -}; - -fn lock(fs: *fs::fs, cachedir: str) (*io::stream | error) = { - // XXX: I wonder if this should be some generic function in fs or - // something - match (os::mkdirs(cachedir)) { - errors::exists => void, - void => void, - e: fs::error => return e, - }; - let lockpath = path::join(cachedir, "manifest.lock"); - defer free(lockpath); - - let logged = false; - for (true) { - match (fs::create(fs, lockpath, 0o644, fs::flags::EXCL)) { - fd: *io::stream => return fd, - (errors::busy | errors::exists) => void, - err: fs::error => return err, - }; - if (!logged) { - fmt::errorfln("Waiting for lock on {}...", lockpath): void; - logged = true; - }; - time::sleep(1 * time::SECOND); - }; - abort("Unreachable"); -}; - -fn unlock(fs: *fs::fs, cachedir: str, s: *io::stream) void = { - let lockpath = path::join(cachedir, "manifest.lock"); - defer free(lockpath); - match (fs::remove(fs, lockpath)) { - void => void, - err: fs::error => abort("Error removing module lock"), - }; + fs::move(ctx.fs, fd.name, mpath)?; }; fn input_finish(in: *input) void = { diff --git a/temp/+linux.ha b/temp/+linux.ha @@ -35,20 +35,22 @@ export fn file( return match (os::create(get_tmpdir(), fmode, oflags)) { // TODO: Add a custom "close" function which removes the named // file - err: fs::error => named(get_tmpdir(), iomode, mode...), + err: fs::error => named(os::cwd, get_tmpdir(), iomode, mode...), s: *io::stream => s, }; }; -// Creates a named temporary file in the given directory. The path to the -// temporary file may be found via the name field of [[io::stream]]. The caller -// is responsible for removing the file when they're done with it. +// Creates a named temporary file in the given directory of the given +// filesystem. The path to the temporary file may be found via the name field of +// [[io::stream]]. The caller is responsible for removing the file when they're +// done with it. // // The I/O mode must be either [[io::mode::WRITE]] or [[io::mode::RDWR]]. // // Only one variadic argument may be provided, if at all, to specify the mode of // the new file. The default is 0o644. export fn named( + fs: *fs::fs, path: str, iomode: io::mode, mode: fs::mode... @@ -71,7 +73,7 @@ export fn named( random::buffer(rand); const id = *(&rand[0]: *u64); const fpath = fmt::bsprintf(pathbuf, "{}/temp.{}", path, id); - match (os::create(fpath, fmode, oflags)) { + match (fs::create(fs, fpath, fmode, oflags)) { errors::exists => continue, err: fs::error => return err, s: *io::stream => return s,