pkulchenko / ZeroBraneStudio

Lightweight Lua-based IDE for Lua with code completion, syntax highlighting, live coding, remote debugger, and code analyzer; supports Lua 5.1, 5.2, 5.3, 5.4, LuaJIT and other Lua interpreters on Windows, macOS, and Linux
http://studio.zerobrane.com/
Other
2.62k stars 518 forks source link

[win] ZBS keeps file handles on some random directory. #1029

Closed moteus closed 4 years ago

moteus commented 4 years ago

My project has directory with a banch of test files. When test is running it creates new temporary directory (and remove old one if it exists) in the same dirs (e.g. test1.1 file creates tmp_test1 directory). After test running ZBS open new file handler pointed to this new directory, but no always to the same one. ZBS keep handler only for one dir. E.g. there exists handler to tmp_test1 and I run test2 - ZBS either close existed one and open new one to the tmp_test2 or just keep handler to the tmp_test1. It is not related to the file watcher because I comment out this code in my ZBS for debug this.

The main problem that I can not remove such dirs until ZBS keeps handlers open.

pkulchenko commented 4 years ago

@moteus, can you try setting filetree.showchanges=false in the config? ZBS tracks changes in the project directory by default (to reflect the filesystem changes in the project tree) and in combination with the wxiwdgets issue I mentioned in https://github.com/pkulchenko/wxlua/issues/52, it may be slow to release watcher holds on (removed) directories.

moteus commented 4 years ago

I can, but as I said I comment out watcher code in the filetree.lua file. Before I did this I saw increasing file handlers every time I expand directory. After I comment there no such effect.

pkulchenko commented 4 years ago

@moteus, ok, I might have misunderstood what code was commented out.

So, you are saying that with the default settings ZBS keeps some of the directories under project tree "locked", but with filetree.showchanges=false everything works as expected? If so, it's a known issue related to the wxwidgets ticket I opened and there is not much I can do about it in the IDE itself. Let me know if you find a workaround or if you think there is something else going on.

moteus commented 4 years ago

I comment code that creates watcher object. After that ZBS blocks only one subdirectory in my test directory. Before that there may be more than one.

pkulchenko commented 4 years ago

Right, this change has the same effect as setting filetree.showchanges=false.

I'm not sure why one directory would still be blocked though, as the watchers are not used anywhere else.

pkulchenko commented 4 years ago

After test running ZBS open new file handler pointed to this new directory, but no always to the same one. ZBS keep handler only for one dir. E.g. there exists handler to tmp_test1 and I run test2 - ZBS either close existed one and open new one to the tmp_test2 or just keep handler to the tmp_test1.

@moteus, did you confirm that the directory is open using the project explorer? The IDE shouldn't scan project (sub-)directories unless they are expanded in the project tree or unless they are searched. Are you doing any of that? I wouldn't expect anything else to trigger the directory processing.

I'll check the logic in the getDir (from util.lua), as all the directories are supposed to be closed when they are scanned, but there may be some exit points that I'm not accounting for. Can you try to reproduce with the following patch and see if you get Open message without a corresponding Close message?

diff --git a/src/util.lua b/src/util.lua
index 640b701e..8837c899 100644
--- a/src/util.lua
+++ b/src/util.lua
@@ -157,6 +157,7 @@ function FileSysGetRecursive(path, recursive, spec, opts)
       if TR then ide:Print(TR("Can't open '%s': %s"):format(path, wx.wxSysErrorMsg())) end
       return true -- report and continue
     end
+    ide:Print("Open", path)

     -- recursion is done in all folders if requested,
     -- but only those folders that match the spec are returned
@@ -204,7 +205,7 @@ function FileSysGetRecursive(path, recursive, spec, opts)
       found, file = dir:GetNext()
     end
     -- wxlua < 3.1 doesn't provide Close method for the directory, so check for it
-    if ide:IsValidProperty(dir, "Close") then dir:Close() end
+    if ide:IsValidProperty(dir, "Close") then ide:Print("Close", path) dir:Close() end
     return true
   end
   while #queue > 0 and getDir(table.remove(queue)) do end

You may want to uncheck Project | Clear Output Window if you need to launch external applications. Can you confirm that it's indeed the IDE holding the file handle?

pkulchenko commented 4 years ago

@moteus, one more patch to test, which adds Close call to all exit points:

diff --git a/src/util.lua b/src/util.lua
index 640b701e..afc54eeb 100644
--- a/src/util.lua
+++ b/src/util.lua
@@ -151,6 +151,11 @@ function FileSysGetRecursive(path, recursive, spec, opts)

   local dir = wx.wxDir()
   local num = 0
+  local function close(dir)
+    -- wxlua < 3.1 doesn't provide Close method for the directory, so check for it
+    if ide:IsValidProperty(dir, "Close") then dir:Close() end
+    return true
+  end
   local function getDir(path)
     dir:Open(path)
     if not dir:IsOpened() then
@@ -174,7 +179,7 @@ function FileSysGetRecursive(path, recursive, spec, opts)
       -- `false` to skip the directory and continue
       -- `nil` to abort the process
       local ondirectory = optondirectory and optondirectory(fname)
-      if optondirectory and ondirectory == nil then return false end
+      if optondirectory and ondirectory == nil then return close(dir) and false end

       if recursive and ismatch(fname..sep, nil, exmasks) and (ondirectory ~= false)
       and (optfollowsymlink or not isSymlink(fname))
@@ -186,7 +191,7 @@ function FileSysGetRecursive(path, recursive, spec, opts)
       end

       num = num + 1
-      if optmaxnum and num >= optmaxnum then return false end
+      if optmaxnum and num >= optmaxnum then return close(dir) and false end

       found, file = dir:GetNext()
     end
@@ -199,13 +204,11 @@ function FileSysGetRecursive(path, recursive, spec, opts)
       end

       num = num + 1
-      if optmaxnum and num >= optmaxnum then return false end
+      if optmaxnum and num >= optmaxnum then return close(dir) and false end

       found, file = dir:GetNext()
     end
-    -- wxlua < 3.1 doesn't provide Close method for the directory, so check for it
-    if ide:IsValidProperty(dir, "Close") then dir:Close() end
-    return true
+    return close(dir) and true
   end
   while #queue > 0 and getDir(table.remove(queue)) do end
pkulchenko commented 4 years ago

After test running ZBS open new file handler pointed to this new directory

Just one more comment on this: I don't see any reason for this to happen if you don't open any files from that directory in the IDE (and seems like you don't, since the directory is created during tests being run). I'm not sure why any (sub-)directories would be scanned/opened there, but the first diff would confirm if any are open from that function (and there are no other places scanning directories).

moteus commented 4 years ago

Sorry. I forgot mention that I also made similar patch for close dir object in the util.lua file. Just test one more time. In the ZBS directory d:\project\***\t expanded and opened d:\project\***\t\targeting.t file. Just after reboot ZBS print to the console

Open    interpreters
Open    spec
Open    packages
Open    C:\Users\alexey\.zbstudio\packages
Open    api
Open    api\lua
Open    D:\project\***
Open    zbstudio\res
Open    D:\project\***\t
Open    D:\project\***\t

And process explorer shows image

In this case I just start ZBS image

May be it relates to the indexing, or may be some path manipulation function opens handler (Win API full of magic :) ) Seems ZBS keep handle to the last dir in the list. After expand another directory ZBS closes old handle and and creates new one.

moteus commented 4 years ago

I also add print for FileGetLongPath

userdata: 029b1400 [wxDir(025fe4d0, 40)]    Open-1  interpreters
userdata: 029b1400 [wxDir(025fe4d0, 40)]    Close
userdata: 028fdd98 [wxDir(025fe460, 40)]    Open-1  spec
userdata: 028fdd98 [wxDir(025fe460, 40)]    Close
userdata: 0295fa50 [wxDir(025fe4a0, 40)]    Open-1  packages
userdata: 0295fa50 [wxDir(025fe4a0, 40)]    Close
userdata: 02994aa8 [wxDir(025fe4f0, 40)]    Open-1  C:\Users\alexey\.zbstudio\packages
userdata: 02994aa8 [wxDir(025fe4f0, 40)]    Close
userdata: 049b97a0 [wxDir(02609808, 40)]    Open-1  api
userdata: 049b97a0 [wxDir(02609808, 40)]    Close
userdata: 049b97a0 [wxDir(02609808, 40)]    Open-1  api\lua
userdata: 049b97a0 [wxDir(02609808, 40)]    Close
userdata: 049a9be8 [wxDir(0264f178, 40)]    Open-1  D:\project\***
userdata: 049a9be8 [wxDir(0264f178, 40)]    Close
userdata: 04eb27c0 [wxDir(0264ecf8, 40)]    Open-2  D:
userdata: 04eb27c0 [wxDir(0264ecf8, 40)]    Close
userdata: 04eb27c0 [wxDir(0264ecf8, 40)]    Open-2  D:\project
userdata: 04eb27c0 [wxDir(0264ecf8, 40)]    Close
userdata: 04eb27c0 [wxDir(0264ecf8, 40)]    Open-2  D:\project\***
userdata: 04eb27c0 [wxDir(0264ecf8, 40)]    Close
userdata: 04eb27c0 [wxDir(0264ecf8, 40)]    Open-2  D:\project\***\t
userdata: 04eb27c0 [wxDir(0264ecf8, 40)]    Close
userdata: 04b36d10 [wxDir(02659580, 40)]    Open-1  zbstudio\res
userdata: 04b36d10 [wxDir(02659580, 40)]    Close
userdata: 04e96a10 [wxDir(026598f0, 40)]    Open-1  D:\project\***\t
userdata: 04e96a10 [wxDir(026598f0, 40)]    Close
userdata: 04f6c158 [wxDir(02dee108, 40)]    Open-1  D:\project\***\t
userdata: 04f6c158 [wxDir(02dee108, 40)]    Close

So in this place all dirs closes and even

moteus commented 4 years ago

I think I found a problem place. It is FileDirHasContent. I rewrite this function like

local function closeDir(dir)
  -- wxlua < 3.1 doesn't provide Close method for the directory, so check for it
  if dir and ide:IsValidProperty(dir, "Close") then
    dir:Close()
  end
end

function FileDirHasContent(path)
  local dir = wx.wxDir()
  dir:Open(path)

  if not dir:IsOpened() then
    return true
  end

  local result = dir:GetFirst("*", wx.wxDIR_DIRS + wx.wxDIR_FILES + wx.wxDIR_HIDDEN)

  closeDir(dir)

  return not not result
end

And there no more extra file handler

PS: I think there no bug in wxWindgets. https://docs.wxwidgets.org/3.0/group__group__funcmacro__file.html#gab136c4ed6c3d71262d12780b723479ce and warning As of 2.5.2, these functions are not thread-safe! (they use static variables). I suspect that this static variable is handle which should be closed by FindClose, but wxWidgets does it implicit in the wxFindNextFile function.

pkulchenko commented 4 years ago

I suspect that this static variable is handle which should be closed by FindClose, but wxWidgets does it implicit in the wxFindNextFile function.

I agree; it does use implicit file handle that is kept open; good catch!

I tweaked the patch a bit, as the original check doesn't search for hidden files:

diff --git a/src/util.lua b/src/util.lua
index 640b701e..62b7be42 100644
--- a/src/util.lua
+++ b/src/util.lua
@@ -58,9 +58,19 @@ function GetPathWithSep(wxfn)
   return wxfn:GetPath(bit.bor(wx.wxPATH_GET_VOLUME, wx.wxPATH_GET_SEPARATOR))
 end

-function FileDirHasContent(dir)
-  local f = wx.wxFindFirstFile(dir, wx.wxFILE + wx.wxDIR)
-  return #f>0
+local function closeDir(dir)
+  -- wxlua < 3.1 doesn't provide Close method for the directory, so check for it
+  if ide:IsValidProperty(dir, "Close") then dir:Close() end
+  return true
+end
+
+function FileDirHasContent(path)
+  local dir = wx.wxDir()
+  dir:Open(path)
+  if not dir:IsOpened() then return false end
+  local found = dir:GetFirst("*", wx.wxDIR_DIRS + wx.wxDIR_FILES
+    + (ide.config.showhiddenfiles and wx.wxDIR_HIDDEN or 0))
+  return closeDir(dir) and found
 end

 -- `fs` library provides Windows-specific functions and requires LuaJIT
@@ -174,7 +184,7 @@ function FileSysGetRecursive(path, recursive, spec, opts)
       -- `false` to skip the directory and continue
       -- `nil` to abort the process
       local ondirectory = optondirectory and optondirectory(fname)
-      if optondirectory and ondirectory == nil then return false end
+      if optondirectory and ondirectory == nil then return closeDir(dir) and false end

       if recursive and ismatch(fname..sep, nil, exmasks) and (ondirectory ~= false)
       and (optfollowsymlink or not isSymlink(fname))
@@ -186,7 +196,7 @@ function FileSysGetRecursive(path, recursive, spec, opts)
       end

       num = num + 1
-      if optmaxnum and num >= optmaxnum then return false end
+      if optmaxnum and num >= optmaxnum then return closeDir(dir) and false end

       found, file = dir:GetNext()
     end
@@ -199,13 +209,11 @@ function FileSysGetRecursive(path, recursive, spec, opts)
       end

       num = num + 1
-      if optmaxnum and num >= optmaxnum then return false end
+      if optmaxnum and num >= optmaxnum then return closeDir(dir) and false end

       found, file = dir:GetNext()
     end
-    -- wxlua < 3.1 doesn't provide Close method for the directory, so check for it
-    if ide:IsValidProperty(dir, "Close") then dir:Close() end
-    return true
+    return closeDir(dir) and true
   end
   while #queue > 0 and getDir(table.remove(queue)) do end

@@ -256,15 +264,14 @@ function FileGetLongPath(path)
   local dirs = fn:GetDirs()
   table.insert(dirs, fn:GetFullName())
   local normalized = vol and volsep and vol..volsep or (path:match("^[/\\]") or ".")
-  local hasclose = ide:IsValidProperty(dir, "Close")
   while #dirs > 0 do
     dir:Open(normalized)
     if not dir:IsOpened() then return path end
     local p = table.remove(dirs, 1)
     local ok, segment = dir:GetFirst(p)
-    if not ok then return path end
+    if not ok then return closeDir(dir) and path end
     normalized = MergeFullPath(normalized,segment)
-    if hasclose then dir:Close() end
+    closeDir(dir)
   end
   local file = wx.wxFileName(normalized)
   file:Normalize(wx.wxPATH_NORM_DOTS) -- remove leading dots, if any

Any reason to use not not result? The result value should already be boolean, right? I also made it return false on failure to open, as I suspect it may happen on some strange network drives and it would be prudent to avoid scanning those then.

Let me know if the patch work for you. Thank you for researching it and providing a patch!

pkulchenko commented 4 years ago

@moteus, I updated the earlier patch to add a check for showhiddenfiles config setting and include hidden files if requested.

moteus commented 4 years ago

Fix looks good. I just do not like code return closeDir(dir) and path I read it as return path only in case of the success dir closing. And it requires to check out closeDir function to really understend that path returns in any case.

pkulchenko commented 4 years ago

@moteus, agree on closeDir semantics. Pushed the updated fix to the master branch.