Attempt to overwrite without race condition

[thatch]

As explained in #332, there was previously a small window of time where the file is deleted before its new contents get written. Because reads don't happen with the lock held, this resulted in an empty-body cache hit when it shouldn't.

Fixes #332

[woodruffw]

Thanks @thatch! I'll try and give this a review in the coming days.

[thatch]

Any luck with testing?

[woodruffw]

Any luck with testing?

Sorry for the delay -- I was out for a bit, and I'm catching up now. I'll do some testing tonight.

[woodruffw]

I did some local testing (on Linux) and didn't hit any issues with this.

I also went through the old _secure_open_write flags and confirmed that we shouldn't be missing anything important here -- the old flags should be irrelevant for our mkstemp use (since it defaults to binary mode and O_NOFOLLOW shouldn't be relevant for fresh temporary files).

As such, this LGTM! I'd like @frostming to also do a comb over it though 🙂

(I didn't test this locally on Windows, but my understanding is that os.replace is equally atomic there.)

[thatch]

Ping @frostming

[woodruffw]

@thatch I'll give @frostming another week or so to chime in, otherwise I'll merge and release this next week.

[woodruffw]

Looks good to me -- I think _write could potentially bail out harder if the underlying write fails rather than complete the rename, but that's worth thinking about separately.