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

feat: wasm module hot reload #168

Merged
merged 5 commits into from
Aug 11, 2021
Merged

feat: wasm module hot reload #168

merged 5 commits into from
Aug 11, 2021

Conversation

zu1k
Copy link
Member

@zu1k zu1k commented Aug 6, 2021

What this PR does:

Let Layotto monitor whether the .wasm file has changed, and reload the .wasm file if there is a change.
Achieve the effect of dynamically replacing .wasm at runtime

Which issue(s) this PR fixes:

Fixes #165

Special notes for your reviewer:

I'm using https://github.com/fsnotify/fsnotify to monitor whether the specified .wasm file has changed. However I found events may be lost in some cases for the principle of fsnotify. The fsnotify is based on the file system and OS features. For example, on GNU/Linux, inotify is taking the info from the inode of the file, and not the file name. If a file is deleted and it is created again, it may have a different inode. I'm trying to watch its parent directory to deal with this case.

When using mv to overwrite the wasm file, it will remove the file first, then rename the origin file. In this case, fsnotify will lost the monitor when got remove event, resulting in subsequent failure to continue hot loading. We won't have this problem with cp. I'm trying to solve this problem by rewatching the file when the event is remove and file still exists.

What's more, I found that mosn encapsulates wasmer but doesn't provide the ability to reload a wasm module, so currently I reload by uninstalling first and then adding, I don't think this is a good way to do it, if possible I would like someone to expose the method by create a PR for mosn.

Does this PR introduce a user-facing change?:

NONE

@mosn-community-bot
Copy link

Hi @zu1k, welcome to mosn community, Please sign Contributor License Agreement!

After you signed CLA, we will automatically sync the status of this pull request in 3 minutes.

zu1k added 2 commits August 7, 2021 07:10
deal with the case that wasm file was created after deleted
@zu1k
Copy link
Member Author

zu1k commented Aug 7, 2021

I'm trying to create a PR for mosn to provide the ability to reload a wasm plugin. See mosn/mosn#1741 for more details.

If the PR merged, we can use a more convenient method to achieve wasm reload, see zu1k@e26d11b for the latest changes.

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2021

Codecov Report

Merging #168 (b12f055) into main (77e0a4b) will decrease coverage by 1.85%.
The diff coverage is 8.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
- Coverage   60.24%   58.38%   -1.86%     
==========================================
  Files          49       50       +1     
  Lines        2065     2146      +81     
==========================================
+ Hits         1244     1253       +9     
- Misses        700      772      +72     
  Partials      121      121              
Impacted Files Coverage Δ
pkg/wasm/factory.go 2.17% <0.00%> (-0.03%) ⬇️
pkg/wasm/watcher.go 8.75% <8.75%> (ø)
...kg/filter/network/tcpcopy/persistence/work_pool.go 90.19% <0.00%> (+3.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77e0a4b...b12f055. Read the comment docs.

@seeflood
Copy link
Member

seeflood commented Aug 7, 2021

Cool,thanks for your contribution!
I did not expect the fsnotify library to have such flaws...

@zu1k zu1k requested a review from zhenjunMa August 11, 2021 10:57
pkg/wasm/watcher.go Show resolved Hide resolved
@zhenjunMa
Copy link
Contributor

@zu1k thank you for your contribute!

@zhenjunMa zhenjunMa merged commit 9a0cdbf into mosn:main Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.wasm module hot reload
4 participants