Support combining .atomic and .withoutOverwriting in Data.write(to:options:) - #2147
Support combining .atomic and .withoutOverwriting in Data.write(to:options:)#2147maxches99 wants to merge 1 commit into
Conversation
|
I opened this before noticing #2011, which targets the same issue — apologies for the duplicate effort. Having read that discussion, two things need correcting here, and one design question is worth settling before this goes further. Correcting my own motivation. I wrote that "the documentation does not state that the two options are mutually exclusive". That is true of the doc comment in this repository, but not of the published API documentation, which @jmschonfeld quoted on #2011:
So the combination is documented as unsupported, and my "source-breaking regression" framing was wrong. I have updated the PR description accordingly. What I think still stands on its own is the narrower point: On @kperryua's points, mapped onto what this PR actually does:
A cheaper alternative, if implementing the semantics is not something the workgroup wants to commit to right now: narrow this to replacing the Either way, if #2011 is the preferred vehicle I am glad to close this and move the Windows handling and tests over there instead. |
This is in fact a common way of handling this. Method options are often chosen statically by the developer in code, and as documented, the two options together are illegal. With that contract, that code should result in an immediate and unmistakable failure, not an error that could be grouped together with other various runtime errors. In the Objective-C implementation of this, an NSInvalidArgumentException is thrown—not for the intention of it being handled of course, but to be a fatal condition indicating the programmer's error.
Saving the
Yes, this is the safer option. It's unacceptable—and potentially represents a security issue—to break an atomicity guarantee and it builds in a TOCTOU race to the algorithm. It may be acceptable if
I don't have a good suggestion for this yet. The behavior before and after the release should definitely be explained in the documentation at the very least, but that's not a very strong way to help developers avoid the pitfall here. I would also appreciate if any changes here were held until after an upcoming re-application of #2077, which heavily modifies this code. This should hopefully happen this week. |
|
Thanks — that all makes sense, and I'll drop the "fatalError is a poor response" framing: for a statically-chosen, documented-illegal option combination, a hard failure is the right contract, and the NSInvalidArgumentException precedent settles it. So I'm no longer proposing the narrower "throw instead of trap" variant; the only justification left for this PR is making the combination actually supported, which removes the trap as a side effect rather than softening it. On the fallbacks, I'll restructure to: renamex_np(RENAME_EXCL) → on ENOTSUP, fall back to the link()/unlink() path on all platforms including Darwin → CocoaError(.featureUnsupported) if that also fails. The non-atomic lstat+rename path goes away entirely, so there's no configuration where the atomicity guarantee is silently weakened. On the deployment target, I don't have a better answer than you do. I'll at least document the pre/post-release behavior in the doc comments as part of this change, with the caveat that it doesn't do much to keep anyone out of the pitfall. Happy to hold until #2077 is re-applied; I'll rebase on top of it once it lands and update this PR then. |
…tions:)
Data.write(to:options:) traps with fatalError("withoutOverwriting is not
supported with atomic") when both options are passed together. The
combination is documented as unsupported, so the trap is a defensible
contract for a statically chosen, invalid pair of options; what this
change does is make the combination supported, which removes the trap as
a side effect rather than softening it.
The data is written to a temporary file first, as it already is for an
atomic write, and the temporary file is then renamed to the destination
in a way that fails rather than replacing an existing file:
- Darwin: renameatx_np(2) with RENAME_EXCL
- If that is unsupported by the file system, and on all other POSIX
platforms: linkat(2) followed by unlinkat(2). link(2) atomically fails
with EEXIST when the destination exists; renameat2(RENAME_NOREPLACE)
is Linux-specific and is not declared by every C library we support.
- Windows: FileRenameInfoEx without FILE_RENAME_FLAG_REPLACE_IF_EXISTS,
and MoveFileExW without MOVEFILE_REPLACE_EXISTING in the cross-volume
fallback
If the destination exists, the write fails with .fileWriteFileExists and
the temporary file is cleaned up.
None of the existing fallbacks in the atomic path are reachable with
.withoutOverwriting, because each of them replaces the destination: the
EINVAL rename-swap for DOS file systems, the EBUSY retry that rewrites
the file non-atomically, and the ERROR_ACCESS_DENIED read-only retry on
Windows. On a file system that supports neither an exclusive rename nor
hard links the write therefore fails with .featureUnsupported, rather
than silently giving up either atomicity or the guarantee that an
existing file is not replaced.
Resolves swiftlang#1098
1e9189e to
98d26cc
Compare
|
Pushed the restructured version, rebased on top of #2161 now that it has landed. Summary of what changed since your review:
Verification: the Data I/O suite passes on macOS both normally and with the |
Support combining
.atomicand.withoutOverwritinginData.write(to:options:).Motivation:
Data.write(to:options:)traps withfatalError("withoutOverwriting is not supported with atomic")when[.atomic, .withoutOverwriting]are passed together.To be precise about what this PR does and does not argue, after the review discussion below: the combination is documented as unsupported ("You can't combine this constant with
atomicbecauseatomicallows the system to overwrite the original file"), and for a pair of options chosen statically in source, an immediate hard failure is a reasonable contract for a documented-invalid combination — the Objective-C implementation raisesNSInvalidArgumentExceptionfor the same reason. So this is no longer a proposal to soften the trap into a thrown error.What it proposes is to make the combination supported, which removes the trap as a side effect rather than weakening it. The combination is well defined and implementable: write to a temporary file, then move it into place with an exclusive rename. Both guarantees hold at once — the destination is never observed half-written, and an existing file is never replaced.
See also #2011, which targets the same issue.
Resolves #1098
Modifications:
The data is written to a temporary file first, as it already is for an atomic write, and the temporary file is then moved to the destination in a way that fails rather than replacing an existing file:
renameatx_np(2)withRENAME_EXCL.ENOTSUP, orEINVALfor the unrecognized flag), and on all other POSIX platforms:linkat(2)followed byunlinkat(2).link(2)atomically fails withEEXISTwhen the destination exists. (renameat2(RENAME_NOREPLACE)is Linux-specific and is not declared by every C library supported here.)FileRenameInfoExwithoutFILE_RENAME_FLAG_REPLACE_IF_EXISTS, andMoveFileExWwithoutMOVEFILE_REPLACE_EXISTINGin the cross-volume fallback.There is no non-atomic fallback. Compared to the previous revision of this PR, the
lstat+renamepath for file systems that support neither an exclusive rename nor hard links is gone; such a write now fails withCocoaError(.featureUnsupported). Silently weakening one of the two guarantees the caller asked for seems worse than reporting that they cannot both be provided.For the same reason, none of the pre-existing fallbacks in the atomic path are reachable with
.withoutOverwriting, since each of them replaces the destination:EINVALrename-swap for DOS file systems,EBUSYretry that rewrites the file non-atomically,ERROR_ACCESS_DENIEDread-only-attribute retry on Windows (withoutREPLACE_IF_EXISTSthe destination is never deleted, so there is nothing to clear).Rebased onto #2161, which rewrote the atomic write path to use directory file descriptors; the rename helper is
renameat-shaped accordingly.Result:
Data.write(to:options:)with[.atomic, .withoutOverwriting]behaves as follows:CocoaError(.fileWriteFileExists), consistent with non-atomic.withoutOverwriting; the existing file is untouched and the temporary file is cleaned up;CocoaError(.featureUnsupported), with the temporary file cleaned up.On the deployment-target question raised in review: I do not have a better answer than was already suggested. Code built against a newer SDK but running on an older OS still traps, and there is no availability annotation that can express "this combination of options became valid in version X". The doc comment on
write(to:options:)now states both the.featureUnsupportedcase and the trap on older releases, which at least documents the pitfall rather than leaving it to be discovered at runtime.Testing:
atomicWriteWithoutOverwritingin the Data I/O suite covers:.fileWriteFileExists, leaves the existing contents untouched, and leaves no temporary file behind;The Data I/O suite passes on macOS, both normally (exercising
renameatx_np) and with therenameatx_npbranch compiled out, so that thelinkatpath used on non-Darwin platforms is exercised locally as well. The.featureUnsupportedpath is not covered by a test: it needs a file system with neither an exclusive rename nor hard links, which the test suite cannot rely on being mounted.