feat(cache): Write temporary files in a dedicated directory #265
Conversation
This is not as clean as it should be because all locations do a lot of path manipulation manually.
src/cache.rs
Outdated
| @@ -323,26 +347,58 @@ pub struct Caches { | |||
|
|
|||
| impl Caches { | |||
| pub fn from_config(config: &Config) -> io::Result<Self> { | |||
| let tmp_dir = config.cache_dir("tmp"); | |||
| if let Some(ref tmp) = tmp_dir { | |||
| if tmp.exists() { | |||
There was a problem hiding this comment.
If the cleanup job starts, won't this get executed and the tmpdir wiped even though symbolicator is still working?
| /// actual location withing the `cache_dir`. | ||
| /// | ||
| /// Just like for `cache_dir` when this cache is disabled this will be `None`. | ||
| tmp_dir: Option<PathBuf>, |
There was a problem hiding this comment.
Is there a situation where one wants to make this configurable? afaict it is not exposed as config param but I think we should avoid exposing it entirely.. I think NamedTempFile::new() is never what you want
There was a problem hiding this comment.
My intention was also not to expose this. You only get to chose a cache directory (or not) and we decide what to do inside of there. It makes me a little sad that you can create a Cache with cache_dir=Some(...) and tmp_dir=None or the other way around. But only code in this module should be able to do that.
Cleanup mode also creates the config and runs regularly during the server process. So we need to do the cleanup as part of server startup explicitly.
| @@ -403,6 +397,20 @@ impl Caches { | |||
| }) | |||
| } | |||
|
|
|||
| /// Clear the temporary files. | |||
There was a problem hiding this comment.
I think we can make a case for emitting an error to Sentry if the tmpdir is not empty on restart
There was a problem hiding this comment.
This should only happen after a crash and there probably have been alerts for this already in that case? I fear this might just lead to duplicate reporting. It's not really a bad failure mode, just a backstop to some files never being cleaned up
The old code did this in the caller, perhaps that was a bit questionable. A lot of this is a bit questionable.
For some reaons I never got filenames and line numbers out of this, so less useful than I was hoping. I suspect I did something wrong though.
flub
force-pushed
the
feat/cache-tmpdir
branch
from
7c81307
to
a04b024
Compare
This gives us filenames and line numbers. Way more useful
flub
changed the title
This is not as clean as it should be because all locations do a lot of
path manipulation manually.