fix(cache): Add back in the marker at the beginning of malformed cache entries #586
Conversation
| // rewinding and then reading to the end of async_file throws bad file | ||
| // descriptor errors, so just reopen the file | ||
| let mut final_file = File::open(&path)?; |
There was a problem hiding this comment.
I’m wondering why though? This reads like a bug in itself.
| @@ -104,12 +104,21 @@ impl CacheStatus { | |||
| } | |||
| CacheStatus::Malformed(details) => { | |||
| file.rewind().await?; | |||
| file.write_all(MALFORMED_MARKER).await?; | |||
| file.write_all(details.as_bytes()).await?; | ||
|
|
||
| let new_len = MALFORMED_MARKER.len().saturating_add(details.len()); |
There was a problem hiding this comment.
file.stream_position() would give the same, and i have a slight preference for it because it doesn't rely on getting the maths right. but i don't mind.
(i think it might also avoid the cast)
| #[tokio::test] | ||
| async fn test_cache_status_write_positive() -> Result<()> { | ||
| let dir = tempdir()?; | ||
| create_dir_all(&dir).unwrap(); |
There was a problem hiding this comment.
surely this is duplicate of the line above?
|
|
||
| // make sure write doesn't change the pre-existing contents of the file | ||
| // rewinding and then reading to the end of async_file throws bad file | ||
| // descriptor errors, so just reopen the file |
There was a problem hiding this comment.
It throws an error because you opened the file in write-only mode: File::create(&path).
OpenOptions::new().read(true).write(true).create(true).open(&path) would fix that
There was a problem hiding this comment.
@Swatinem answer to your question ^
| let mut final_file = File::open(&path)?; | ||
| let mut contents = Vec::new(); | ||
| final_file.read_to_end(&mut contents)?; |
There was a problem hiding this comment.
let contents = std::fs::read(&path).unwrap() could replace this entire block.
| let sync_file = File::create(&path)?; | ||
| let mut async_file = tokio::fs::File::from_std(sync_file); | ||
| async_file.write_all(b"beep").await?; | ||
| async_file.rewind().await?; |
There was a problem hiding this comment.
I'd not even rewind, status.write() should handle that
|
|
||
| async_file | ||
| .write_all( | ||
| b"i'm a little teapot short and stout here is my handle and here is my spout", |
Primarily fixes a regression from the shared cache PR (#581) which was no longer writing the sentinel/"header" for malformed entries in caches.
Also includes a minor fix that ensures that other non-positive entries in the cache are truncated in case they are overwriting some other content that is slightly longer than their new value. This is highly unlikely to occur, but this is added in as a safeguard if this does ever occur.
This also renames the helper because there's no need for the user to know about the existence of markers at the abstraction level where that would be invoked.
Unit tests have been written because they would have caught this error assuming that they weren't deleted or commented out during the original function's refactor. Proper integration tests would most likely test
computeitself, which is a pretty complicated function to test.