Not removing sleep timer when deleting a thread?

[aviramc]

Hi!

Debugging an issue that I have with my Lua code, I ran across this issue - the function ngx_http_lua_del_thread doesn't remove the sleep timer of the thread in case the thread waits for sleep.

Is this really a bug? And in a more general perspective - shouldn't ngx_http_lua_del_thread call coctx->cleanup for cleaning up any action in which it is stuck?

I've ran across this bug when debugging an issue that might also be interesting - I have a Lua thread that sleeps, and under certain conditions I want the request to end without waiting for this thread to end, so I call ngx.exit. This works okay, but under a stress of roughly 1000 requests per seconds, it crashes. The core dump shows various location in each crash (sometimes in Nginx code and sometimes in LuaJIT), and this looks like a memory corruption that might occur if, for example, a timer event of an already finished request would occur...

Regards
Aviram

[agentzh]

No, it is ngx_http_lua_finalize_coroutines's responsibility to call coctx->cleanup (so as to remove pending sleep timers and etc).

When you call ngx.exit() on the Lua land to abort all the pending threads in the current request, it will trigger calling ngx_http_lua_handle_exit, which then calls ngx_http_lua_request_cleanup that again calls ngx_http_lua_finalize_coroutines.

The crash looks like a bug in ngx_lua. It'll be great if you can prepare a minimized example that can help reproduce the issue on my side so that I can fix it much more easily.

BTW, which version of nginx, ngx_lua, and LuaJIT are you using?

Thank you for the report!

[agentzh]

@aviramc Any updates on this issue?

[aviramc]

Hello!

Unfortunately, I haven't had time to reproduce this, I'll do it soon.
When I try it, I'll try to reproduce with the latest version of ngx_lua, back then I used 0.8.1 I think.

[agentzh]

@aviramc Any updates regarding this issue?

[aviramc]

@agentzh , sorry for the delay in the response. This was a bug in a code change I've made (related to the bug fixed in commit 47836ce).

So this is a non-issue.

[agentzh]

@aviramc Cool! Good to know. Then I'm closing this :)