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

Fix garbage collection of scandir reqs #696

Merged
merged 3 commits into from
Mar 2, 2024
Merged

Conversation

squeek502
Copy link
Member

@squeek502 squeek502 commented Mar 1, 2024

This is one possible approach to solving this. I'd like to test it a bit more and reproduce the crash in #694 (comment) to make sure this fixes it before merging.


Return a userdata wrapper around the uv_req_t to allow it to be garbage collected before the program exits. Previously, the returned userdata held a reference to itself in the Lua registry, meaning it would never be able to be garbage collected until the process ended.

This reverts commit 0e4a895, which attempted a workaround for the same underlying problem, but introduced a use-after-free.

Return a userdata wrapper around the uv_req_t to allow it to be garbage collected before the program exits. Previously, the returned userdata held a reference to itself in the Lua registry, meaning it would never be able to be garbage collected until the process ended.

This reverts commit 0e4a895, which attempted a workaround for the same underlying problem that introduced a use-after-free.
Copy link
Member

@zhaozg zhaozg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done

@zhaozg
Copy link
Member

zhaozg commented Mar 2, 2024

reproduce before cmp with https://github.com/FelipeLema/cmp-async-path.

image

After with this PR, it works.

image

@squeek502
Copy link
Member Author

Thanks! Minimal reproduction:

local uv = require('luv')

local entries = assert(uv.fs_scandir('.'))

local work
work = assert(uv.new_work(function(_entries)
  local _uv = require('luv')
  while true do
    if not _uv.fs_scandir_next(_entries) then break end
  end
end, function() end))

work:queue(entries)

uv.run()

@squeek502 squeek502 merged commit 7233e6d into luvit:master Mar 2, 2024
11 of 12 checks passed
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 this pull request may close these issues.

2 participants