util: Remove code to cache datadir #16255
Conversation
|
Will reopen after #16252 |
|
Concept ACK |
src/util/system.cpp
Outdated
There was a problem hiding this comment.
happy to see this code simplified, it has been a frequent source of errors in the past
maflcko
force-pushed
the
1906-utilNoPath
branch
4 times, most recently
from
fa1c363 to
aaaa660
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
A side-effect of caching was that this code was only run the first call to if (fs::create_directories(path)) {
// This is the first run, create wallets subdirectory too
fs::create_directories(path / "wallets");
}(this not only results in system call spam, but also a change in behavior if, for some reason, the datadir disappears) |
that's good? @laanwj wouldn't it be better to just call |
That definitely isn't good, if the datadir disappears while running you want the process to die as soon as possible and not mess with creating directories.
Yes that would be better, however this is very hard to reason about, and review, with the complexity of our startup sequence. Maybe better to leave for a future PR, which is why I suggested the most straightforward solution. |
maflcko
force-pushed
the
1906-utilNoPath
branch
2 times, most recently
from
75d7c6c to
facf965
Compare
|
It is indeed weird to write to disk in a method, whose name implies it is a simple getter. Added a guard to the disk access for now, but this should be improved in the future. (See also "Fix datadir handling" #15864) |
|
I think this is not worth it, so giving up on this for now. Someone should move out the |
fa69c3e util: Explain why the path is cached (MarcoFalke) Pull request description: The rationale for caching the datadir is given as ``` // This can be called during exceptions by LogPrintf(), so we cache the // value so we don't have to do memory allocations after that. ``` Since 8c2d695, the debug log location is actually cached itself in `m_file_path`. So explain that the caching is now only used to guard against disk access on each call. (See also #16255) ACKs for top commit: promag: ACK fa69c3e. laanwj: ACK fa69c3e ryanofsky: utACK fa69c3e. Good cleanup. Previous comment was confusing, and definitely not helpful if outdated. Tree-SHA512: 02108c90026d6d7c02843aaf59a06b4e1fa63d5d4378bb7760f50767efc340dc94c259bf7afb32fa4d47952b48a4e91798d1e0ddc1b051d770405e078636793a
fa69c3e util: Explain why the path is cached (MarcoFalke) Pull request description: The rationale for caching the datadir is given as ``` // This can be called during exceptions by LogPrintf(), so we cache the // value so we don't have to do memory allocations after that. ``` Since 8c2d695, the debug log location is actually cached itself in `m_file_path`. So explain that the caching is now only used to guard against disk access on each call. (See also bitcoin#16255) ACKs for top commit: promag: ACK fa69c3e. laanwj: ACK fa69c3e ryanofsky: utACK fa69c3e. Good cleanup. Previous comment was confusing, and definitely not helpful if outdated. Tree-SHA512: 02108c90026d6d7c02843aaf59a06b4e1fa63d5d4378bb7760f50767efc340dc94c259bf7afb32fa4d47952b48a4e91798d1e0ddc1b051d770405e078636793a
5 participants
The rationale for caching the datadir is given as
However, since 8c2d695, the debug log location is actually cached itself in
m_file_path.So remove the now-useless caching.