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

fs_scandir iterator causes memory leak with early break #600

Closed
gpanders opened this issue May 28, 2022 · 2 comments · Fixed by #601
Closed

fs_scandir iterator causes memory leak with early break #600

gpanders opened this issue May 28, 2022 · 2 comments · Fixed by #601

Comments

@gpanders
Copy link

Using fs_scandir and fs_scandir_next as an iterator results in a memory leak if the iterator is not fully drained. Example:

local luv = require 'luv'

local function dir(d)
  return function(fs)
    return luv.fs_scandir_next(fs)
  end, luv.fs_scandir(d)
end

for name in dir('.') do
  print(name)
  break
end

This results in a memory leak. This happens because the uv_fs_req is only cleaned up when the iterator reaches a UV_EOF:

luv/src/fs.c

Lines 600 to 605 in c51e705

if (ret == UV_EOF) {
luv_cleanup_req(L, (luv_req_t*)req->data);
req->data = NULL;
uv_fs_req_cleanup(req);
return 0;
}

However, when the iteration loop breaks early this condition is never hit and uv_fs_req_cleanup is never called.

It does not seem possible to manually cleanup the fs handles either: fs:close() does not exist.

@gpanders gpanders changed the title Iterator fs_scandir causes memory leak with early break fs_scandir iterator causes memory leak with early break May 28, 2022
@squeek502
Copy link
Member

Thanks for the report, I'll look into it.

@squeek502
Copy link
Member

Here's an even simpler reproduction:

local uv = require('luv')
local req = uv.fs_scandir('.')

As pointed out in the OP, we currently only handle cleanup when we fully iterate with scandir_next.

squeek502 added a commit to squeek502/luv that referenced this issue May 31, 2022
…scandir_next`

Now, we have a specialized metatable ("uv_fs") with a gc function that's only used for scandir's uv_fs_t reqs, so that it will always be cleaned up automatically. This also means that once fully iterated, the req is no longer immediately cleaned up and so any subsequent calls to `fs_scandir_next` will just return `nil` (whereas previously it would have errored out in `luv_check_fs`).

Closes luvit#600
squeek502 added a commit that referenced this issue May 31, 2022
…scandir_next` (#601)

Now, we have a specialized metatable ("uv_fs") with a gc function that's only used for scandir's uv_fs_t reqs, so that it will always be cleaned up automatically. This also means that once fully iterated, the req is no longer immediately cleaned up and so any subsequent calls to `fs_scandir_next` will just return `nil` (whereas previously it would have errored out in `luv_check_fs`).

Closes #600
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants