fix issue 5350, check file lock before reload model #5351
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5351 +/- ##
=======================================
Coverage 73.97% 73.97%
=======================================
Files 1019 1019
Lines 190005 190031 +26
Branches 20455 20455
=======================================
+ Hits 140550 140580 +30
+ Misses 43905 43903 -2
+ Partials 5550 5548 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -105,13 +105,37 @@ public override ITransformer GetModel() | |||
| return _model; | |||
| } | |||
|
|
|||
| public bool IsFileLocked(string filePath) | |||
| { | |||
There was a problem hiding this comment.
This seems okay, but it is better to return the file handle directly instead of returning bool and trying to reopen the file.
See the second answer here for a better solution: https://stackoverflow.com/questions/50744/wait-until-file-is-unlocked-in-net
#Resolved
There was a problem hiding this comment.
Thanks, updated.
| Thread.Sleep(50); | ||
| using (var fileStream = File.OpenRead(_filePath)) | ||
| var fs = WaitForFile(_filePath, FileMode.Open, FileAccess.Read, FileShare.Read); | ||
| if (fs == null) |
There was a problem hiding this comment.
Since you have a repro for this issue, can you please check it in as a test for this bug?
There was a problem hiding this comment.
There is no easy way to test this, this repro requires to write a web api and call webapi in certain order and then check the output. We need to introduce some web test framework to do this.
There was a problem hiding this comment.
You don't need a web test to use Microsoft.Extensions.ML. You can use it from any test project. We already have some tests here:
https://github.com/dotnet/machinelearning/tree/master/test/Microsoft.Extensions.ML.Tests
There was a problem hiding this comment.
@eerhardt Thanks for the pointer. @frank-dong-ms Can you please add a test for this issue?
There was a problem hiding this comment.
Sure, thanks Eric, will do that in separate PR.
ghost
locked as resolved and limited conversation to collaborators
fix issue #5350
The issue here is we use a file system watcher to watch model file change and reload the new model file automatically. The problem here is when we try to load the new model file this file is still locked thus cause the reload to fail.
Previously we are wait for 50 milliseconds before reload the new model file but that fails to work sometimes so here change the strategy to consistently check whether the file can be reload every 50 milliseconds for at most 100 times (which is roughly at total 5 seconds). If we still can't reload new model file after 5 seconds, throw IOException.