fix(engine-dylib): do not keep temp file and delete it automatically #2547
Conversation
…asmerio#2501 Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
lib/engine-dylib/src/artifact.rs
Outdated
| @@ -58,6 +58,12 @@ pub struct DylibArtifact { | |||
| frame_info_registration: Mutex<Option<GlobalFrameInfoRegistration>>, | |||
| } | |||
|
|
|||
| impl Drop for DylibArtifact { | |||
| fn drop(&mut self) { | |||
| std::fs::remove_file(&self.dylib_path).expect("cannot delete the temporary artifact"); | |||
There was a problem hiding this comment.
This will drop the file always, however there is only one scenario where we should do it.
deserialize (which creates a temporal file and then callsdeserialize_from_file_unchecked) should delete the temporal file, however deserialize_from_file (which doesn't create a temporal file) should not delete the file (since it was provided by the user), we can provably have a new field specifying if the file is temporal or not and delete it on such case.
There was a problem hiding this comment.
Ok got it thank you, btw what's your policies regarding to error in this case ? Do you prefer to panic as I do here or just display a log ?
There was a problem hiding this comment.
I think it might be better to gracefully dismiss the error in case the worst case scenario is reached, so logging seems like a better option than a panic.
There was a problem hiding this comment.
That's what I think too. Updated, thanks :)
There was a problem hiding this comment.
That's great, will review now!
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
|
bors r+ |
2547: fix(engine-dylib): do not keep temp file and delete it automatically r=syrusakbary a=bnjjj close #2501 # Description This PR plan to fix the issue #2501 I know the implementation using Drop trait is not the best way to implement it. Maybe I need more knowledge about engine lifecycle or adding a trait method to clean any artifacts we create when using an engine. Feel free to give me more details about how I could implement this in a better way. Co-authored-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
|
Build failed: |
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
|
Sorry I had to add a feature on dependency |
|
No worries. bors r+ |
close #2501
Description
This PR plan to fix the issue #2501
I know the implementation using Drop trait is not the best way to implement it. Maybe I need more knowledge about engine lifecycle or adding a trait method to clean any artifacts we create when using an engine. Feel free to give me more details about how I could implement this in a better way.