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

atexit() is probably not a good way to clean up Lua VM #712

Open
M1NGS opened this issue Jul 17, 2024 · 6 comments
Open

atexit() is probably not a good way to clean up Lua VM #712

M1NGS opened this issue Jul 17, 2024 · 6 comments
Labels

Comments

@M1NGS
Copy link

M1NGS commented Jul 17, 2024

Hi everyone
I working in Lazarus(freepascal) with luajit & luv, When i try to staitc link luajit and luv in my program, I got some trouble.
luv work well in dll mode on windows/mingw, because atexit() works on dll instance, The main thread's memory will not be written.
but if i build luv as a static lib and preload it in Lua/LuaJIT static lib, and link them to freepascal. when i require luv then get a runtime error, cause the halt() mechanism of freepascal compiler is incompatible with atexit().

I think other languages ​​that can statically link c libs may have this problem.

I thought we may need explicit call a method such as uv.clearVM() before terminate threads that required luv?

@M1NGS M1NGS changed the title atexit() is probably not a good way to clean up your VM atexit() is probably not a good way to clean up Lua VM Jul 17, 2024
@truemedian
Copy link
Member

You beat me to finally writing up an issue to report this. I have encountered this same issue when using luv with musl libc, because unlike glibc musl does not handle shared libraries specially with regards to atexit hooks.

I'd even go farther to assert that using atexit in luv is invalid for the luv module because it is guaranteed to be dynamically linked at runtime and, more importantly unlinked at runtime when the Lua state is closed, which will always happen before a normal exit.

This problem doesn't manifest in most scenarios because Windows and glibc (likely also apple, but cannot verify) handle atexit calls in a shared library to be registered instead right before the shared library is unloaded.

A likely more correct implementation would use __attribute__((destructor)) on luv_work_cleanup to always be called when the luv module right before the module is unloaded; however this is a GNU-specific attribute and we would need to find an alternative solution for other compilers if we wish it maintain support for them.

@truemedian truemedian added the bug label Jul 18, 2024
@M1NGS
Copy link
Author

M1NGS commented Jul 18, 2024

I'd even go farther to assert that using atexit in luv is invalid for the luv module because it is guaranteed to be dynamically linked at runtime and, more importantly unlinked at runtime when the Lua state is closed, which will always happen before a normal exit.

nod, it doesn't make sense on the main thread or static link program. only scenario of available is when the user triggers gc normally in work thread.
For example, do uv = nil or lua_close() before close work thread.

static int lj_cf_package_unloadlib(lua_State *L)
{
  void **lib = (void **)luaL_checkudata(L, 1, "_LOADLIB");
  if (*lib) ll_unloadlib(*lib);
  *lib = NULL;  /* mark library as closed */
  return 0;
}

LUALIB_API int luaopen_package(lua_State *L)
{
  int i;
  int noenv;
  luaL_newmetatable(L, "_LOADLIB");
  lj_lib_pushcf(L, lj_cf_package_unloadlib, 1);
  lua_setfield(L, -2, "__gc");

@truemedian
Copy link
Member

I was recently looking through the openssl configure options and unsurprisingly they encounter this same problem. Their solution seems to have been to dlopen themselves (essentially, leak a reference to the library) to ensure it stays mapped until the program terminates.

This isn't really the ideal solution for luv, because it means once loaded you wouldn't be able to get rid of luv, but it is a solution.

@M1NGS
Copy link
Author

M1NGS commented Dec 18, 2024

Maybe try making some changes? Like calling luv_work_cleanup explicitly. I could submit a pull request, but I'd like to get a thorough discussion with the project maintainers before do that.

@truemedian
Copy link
Member

As long as there is a way to clean up resources and it gets called it's supposed to on all platforms under all scenarios it doesn't matter too much about how it happens. It just needs to.

I looked into just moving this check into the __gc of loop and it seems this might be the best idea. The only cleanup that needs to happen is related to Lua states used by the work callbacks, which means once the loop is done running (the only time we can safely de-initialize it anyway) we are safe to free these.

Not sure why atexit was chosen here in the first place when __gc exists.

@M1NGS
Copy link
Author

M1NGS commented Dec 18, 2024

It's a good idea to let Lua's gc handle it, and it is also forward compatible, about atexit, maybe they didn't foresee today's situation at that time.
just do it please!

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

No branches or pull requests

2 participants