Use the non-throwing std::filesystem overloads in os::Utils - #271
Closed
georges-berenger wants to merge 4 commits into
Closed
Use the non-throwing std::filesystem overloads in os::Utils#271georges-berenger wants to merge 4 commits into
georges-berenger wants to merge 4 commits into
Conversation
meta-cla
Bot
added
the
CLA Signed
Contributor
|
@georges-berenger has exported this pull request. If you are a Meta employee, you can view the originating Diff in D116692071. |
georges-berenger
force-pushed
the
export-D116692071
branch
4 times, most recently
from
August 20, 2026 02:49
f4cc6da to
e488a7f
Compare
Summary:
`AsyncDiskFileChunk.cpp` held 8 of the 10 `throw` statements in VRS core, all
`std::runtime_error`. Each becomes what it actually was — a failure the caller can act on
turns into an error code, a broken invariant turns into an abort. Nothing is downgraded to
log-and-continue.
- **5 aio-contract violations in `AsyncBuffer::SigEvNotifyFunction` -> `XR_CHECK`.** It is
registered with `SIGEV_THREAD`, so it runs on a thread the C library created: the
exceptions were already unwinding into C frames and hitting `std::terminate` with no
diagnostic. `XR_CHECK` aborts the same way but logs the condition and the offending
`aio_return`/`aio_error` values. `XR_CHECK(false, ...)` rather than `XR_FATAL_ERROR`,
which the OSS shim defines as `UNREACHABLE_CODE()` — neither logs nor aborts.
- **Unaligned capacity in `AlignedBuffer`'s constructor -> `XR_CHECK`.** Already
unreachable: `init_parameters()` rejects a non-conforming `buffer_size` with
`DISKFILE_INVALID_STATE` before any buffer is built.
- **`AlignedBuffer::add()` -> a VRS error code, and a narrower notion of failure.** It
returned `ssize_t` — a byte count, or -1 covering two unrelated conditions, one of which
used to be the throw. It is now
`int add(const void*, size_t, size_t& outCopiedSize)` returning `SUCCESS` or
`DISKFILE_INVALID_STATE`, propagated with `IF_ERROR_RETURN` instead of translating a
sentinel.
Only a buffer with no storage is an error. A *full* buffer is not: it is the normal end
state of a successful fill, so it reports `SUCCESS` with `outCopiedSize == 0`. Likewise
`size == 0` is a no-op rather than a precondition violation — the old `assert(size)` is
dropped, since the implementation always handled it correctly and appending nothing is
not a programming error. The one caller is unaffected: it checks `full()` after every
`add()` and either queues-and-swaps or pwrites-and-clears, so the buffer always has room
on entry and every `add()` of a non-zero size copies at least one byte.
- **Allocation failure -> a real `ENOMEM`.** This was a bug. `alloc_write_buffers()` has
`if (!buffer) { return ENOMEM; }`, which never fired: `make_unique` doesn't return null,
the constructor threw, so an OOM escaped `create()`/`open()` — both `int`-returning — out
of the public `WriteFileHandler` API. Constructors are now protected behind
`AlignedBuffer::make()` / `AsyncBuffer::make()`, which return `nullptr` on failure. That
makes the failure impossible for a caller to forget, and makes the existing null check
live code doing what its author intended.
Nothing reachable from `close()` can throw now, so `~AsyncDiskFileChunk()`'s `try`/`catch`
is dead and removed; `close()`'s error is logged instead of silently dropped. The file is
exception-free.
Differential Revision: D116692076
Summary: `printTags()` parsed the `capture_time_epoch` file tag with `stoul()` and no guard. That value comes out of the VRS file, so any file carrying a non-numeric or oversized `capture_time_epoch` made `std::invalid_argument` / `std::out_of_range` escape a `void` printing function, straight out of the public `RecordFileInfo::printOverview()`. Nothing on the path catches, so the process aborts rather than reporting an error — `vrslist`, `vrsplayer`, anything printing file info. Now uses `helpers::readUInt64()`, built on `std::from_chars`: never throws, rejects overflow and trailing garbage. A tag that doesn't parse prints verbatim without the decoded date, exactly as an epoch below the existing 1000000 sanity floor already did. Reviewed By: zakbain Differential Revision: D116692075
Summary: Pins down what the two functions the next diff touches currently do, before switching them off the throwing `std::filesystem` overloads. Adds to `os/test/UtilsTest.cpp`: `listDir()` on a missing path and on a path that is a file both return empty; `getLinkedTarget()` on a regular file and on a missing path both return false with `outLinkedPath` left as the input; and it resolves a real symlink. The symlink test and the `<unistd.h>` include it needs are both gated on `!IS_WINDOWS_PLATFORM() && !IS_ANDROID_PLATFORM()`: Windows has no symlinks, and Android's temp folder may not permit creating them, matching how this file already guards its other filesystem tests. The expected target is built from an already-canonical folder, because `getLinkedTarget()` canonicalizes every path component: on macOS the temp folder sits under /var, itself a symlink to /private/var, so comparing against a raw `pathJoin` result fails there. The broken-symlink case is deliberately *not* here. It only holds once the OSS implementation stops throwing, so it lands with that change in the next diff rather than breaking the OSS CMake build in between. Differential Revision: D116692079
Summary: `os/Utils.cpp` uses the `std::error_code` overloads of `std::filesystem` almost everywhere already. Two functions did not: - `getLinkedTarget()` (OSS build): a broken symlink made `fs::canonical()` throw out of a `bool`-returning function. The `Utils_fb.cpp` twin wrapped the same logic in a `try`/`catch`, so only the OSS build was exposed. - `listDir()`: `fs::directory_iterator(dir)` throws when the directory can't be opened, and `operator++` throws on a mid-iteration error. The `isDir()` pre-check closed neither gap — the directory can vanish between check and open. Both now take the `error_code` overloads, and `listDir()` iterates with `it.increment(ec)` like `os/FileList.cpp` does, so the whole traversal is non-throwing. The `_fb` `try`/`catch` goes away with it, putting the two implementations back in sync. Carries the broken-symlink test rather than the parent diff, because that is the case the OSS implementation used to throw on: before this change it would abort the OSS `test_vrs_os` build instead of failing cleanly. Not addressed: `fs::temp_directory_path()` in `getOsTempFolder()`. Picking a fallback when the OS has no temp folder is a product call, and unlike these two it is not reachable from file content or a caller-supplied path. Differential Revision: D116692071
georges-berenger
force-pushed
the
export-D116692071
branch
from
August 20, 2026 03:09
e488a7f to
ba93658
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
os/Utils.cppuses thestd::error_codeoverloads ofstd::filesystemalmost everywherealready. Two functions did not:
getLinkedTarget()(OSS build): a broken symlink madefs::canonical()throw out of abool-returning function. TheUtils_fb.cpptwin wrapped the same logic in atry/catch, so only the OSS build was exposed.listDir():fs::directory_iterator(dir)throws when the directory can't be opened, andoperator++throws on a mid-iteration error. TheisDir()pre-check closed neither gap —the directory can vanish between check and open.
Both now take the
error_codeoverloads, andlistDir()iterates withit.increment(ec)like
os/FileList.cppdoes, so the whole traversal is non-throwing. The_fbtry/catchgoes away with it, putting the two implementations back in sync.
Carries the broken-symlink test rather than the parent diff, because that is the case the
OSS implementation used to throw on: before this change it would abort the OSS
test_vrs_osbuild instead of failing cleanly.
Not addressed:
fs::temp_directory_path()ingetOsTempFolder(). Picking a fallback whenthe OS has no temp folder is a product call, and unlike these two it is not reachable from
file content or a caller-supplied path.
Differential Revision: D116692071