Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Temp files are not removed in dylib engine #2501

Closed
xofyarg opened this issue Aug 3, 2021 · 4 comments · Fixed by #2518 or #2547
Closed

Temp files are not removed in dylib engine #2501

xofyarg opened this issue Aug 3, 2021 · 4 comments · Fixed by #2518 or #2547
Assignees
Labels
bug Something isn't working 📦 lib-engine-dylib About wasmer-engine-dylib

Comments

@xofyarg
Copy link
Contributor

xofyarg commented Aug 3, 2021

Describe the bug

dylib creates tempfile to be able to "dlopen" later, but those files are not removed later. This could be an issue for a long running program which constantly (re)loads wasm files, occupying lots of unnecessary disk spaces.

Steps to reproduce

Use dylib engine to load a wasm file, then check /tmp for temp files.

Expected behavior

Temp file is deleted when not being used.

Actual behavior

Temp file persists even program restarts.

Additional context

@xofyarg xofyarg added the bug Something isn't working label Aug 3, 2021
@Hywan Hywan added the 📦 lib-engine-dylib About wasmer-engine-dylib label Aug 12, 2021
@Hywan Hywan self-assigned this Aug 12, 2021
@Hywan
Copy link
Contributor

Hywan commented Aug 12, 2021

Thanks for reporting the issue. I confirm the bug. I've written a fix in #2518.

bors bot added a commit that referenced this issue Aug 13, 2021
2518: fix(engine-dylib) Remove temporary file used to creating an artifact r=syrusakbary a=Hywan

# Description

When creating an artifact, a first temporary file is created and kept
to prevent the file from being removed by the system. This patch
removes that temporary file manually whatever the linker fails or not.

Fixes #2501.

# Review

- [x] Add a short description of the change to the CHANGELOG.md file


Co-authored-by: Ivan Enderlin <[email protected]>
@bors bors bot closed this as completed in b72c6f5 Aug 13, 2021
@vavrusa
Copy link
Contributor

vavrusa commented Aug 25, 2021

I think this is still a problem, the #2518 fixes the object file cleanup, but

pub unsafe fn deserialize(
still leaks the temporary file for the dylib.

@syrusakbary
Copy link
Member

syrusakbary commented Aug 25, 2021

Good catch @vavrusa!

@syrusakbary syrusakbary reopened this Aug 25, 2021
@vavrusa
Copy link
Contributor

vavrusa commented Aug 25, 2021

There's a related issue here (and below)

file.write(&obj_bytes).map_err(to_compile_error)?;
(write instead of write_all), and here (named temp file leaked again). I think you don't have to leak it in any of these cases, the libloading will dlopen the file for r+x, so you can remove the file (and get rid of the r+w mapping from to the leaked tempfile handle, which is not safe anyway). The file will get removed from fs immediately, and the r+x will disappear after the library is closed with dlclose.

bnjjj added a commit to bnjjj/wasmer that referenced this issue Aug 31, 2021
bnjjj added a commit to bnjjj/wasmer that referenced this issue Aug 31, 2021
bors bot added a commit that referenced this issue Sep 1, 2021
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 <[email protected]>
@bors bors bot closed this as completed in 378550a Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-engine-dylib About wasmer-engine-dylib
Projects
None yet
4 participants