Defunct processes

[khaneliman]

Been working on migrating config to use sbar lua and see huge performance issues while going through development. Had numerous times i've had to force restart computer because it locks up while restarting sketchybar over and over from config changes. I recently woke my computer up and computer was unresponsive and had to force restart. I saw a bunch of defunct processes in btop and was curious if it might be an issue with how the api is managing all the child processes from the various subscriptions or if it's just my lack of understanding of lua in my migration of my existing config https://github.com/khaneliman/khanelinix/pull/81. I was using latest release version, going to try building from master branch.

[FelixKratz]

Thats odd, in theory, the lua process should be killed as well in a reload. Could be that there is some edge case where this doesnt work. Note that there is only ever one process/thread doing the event handling, such that os.execute or io.popen will block all event handling. This is why the lua callbacks should not take too long to execute. For long lasting calls (as brew update/brew upgrade etc) regular async scripts should be used (as before via the script property or by using sbar.exec). See also #5 for more discussion about this.

I will see if I can repro this issue.

[khaneliman]

Hmm.. alright. So issue could be caused from converting all my scripts to lua event handle subscriptions. I assumed it would be a faster communication, but if they are all single threaded blocking that would be much slower than just invoking my bash scripts. Looking at sbar.exec it just invokes a sh for the command. Is it possible to expand that to support different shells so maybe we could forward lua to be run in sbar.exec async?

[FelixKratz]

Yeah, well, the communication is much faster. Problem is that we have a global state, which we need to protect in lua. I.e. it is possible to access and set global variables in the lua callback functions such that it is not possible to do any callback handling async in lua. We can however do fork->exec from C, which is exactly what sbar.exec does. So this is truely async and will not block the main thread.

I had an idea however, which would be nice I think. The problem seems to be that calling some program through shell is often too slow, so what could be done is to add a lua completion handler to those shell scripts.

Essentially something like this:

sbar.exec("brew outdated", function(result)
  print(result)
end)

Now what we do in C behind the courtains is the following:

• We spawn a new thread
• The new thread executes the command completely async
• once the command has returned, we post an event into the main thread CFRunLoop containing the result
• The runloop eventually calls the completion handler with the result as an argument, which can then be handled in lua.

The problem is that only the result variable will be reliably accessible in the completion handler because it is essentially arbitrary at which point the RunLoop calls the function. So none of the other variables from the scope of sbar.exec will be available.

This adds quite some complexity but I think it really is the best way to keep the event callback thread from being blocked by os.execute or io.popen

[khaneliman]

This adds quite some complexity but I think it really is the best way to keep the event callback thread from being blocked by os.execute or io.popen

Yeah, I do like this idea of a callback to support these types of commands. I assume you were saying you still need to implement the callback or is that possible currently? I tried implementing this and I see the console output of the commands being run but not seeing the function getting hit.

[FelixKratz]

No its not implemented yet, this was just an idea.

[khaneliman]

Alright, i'm going to replace all the os.executes i have with sbar.exec and the io.popen with my existing bash scripts, for now, to get rid of the blocking i'm seeing. But, I do like the idea of using lua for all the logic and just sbar.exec to run the commands to fetch data in a new thread.

[FelixKratz]

I quickly hacked this together and it seems to work just as intended: https://github.com/FelixKratz/SbarLua/commit/19a66adb76ef08ec4d2ce906cbf752d1e0560b81

[khaneliman]

Sweet, I'll try updating code to use that and see if it works for me. Appreciate the quick help!

[khaneliman]

Seems to be a lot more responsive with the bar and I appreciated the json conversion to lua table for my wttrbar usage. Thanks!

I still see a handful of defunct processes while using Sbarlua, must just be related to how sbarlua is spawning the external processes. My windowmanager process kept locking up my computer still and I think that issue might be related to the hotload. I disabled hotloading and the windowmanager process appears to not runaway with usage. knocks on wood Before, with hotload enabled, I had to kill windowmanager to get responsiveness back. I also would manually restart sketchybar when i'd see in my logs that it appeared to be reinitializing nonstop and windowmanager cpu usage and memory kept ballooning.

EDIT: New issue after disabling hotload seems to be runaway process spawning issue, though. Eventually, i see unrelated processes start getting killed by the OS and it wont let me launch new processes until I restart computer.

[shajra]

I like the direction that sbar.exec has gone. Can we make this API support exit codes? Or maybe I need to handle exit codes as part of the command? I can do that... wrap it in an if-then-else to always get parseable output. Maybe a little circuitous, because all the information I need is already in the exit code.

[shajra]

Okay, so I went from a configuration that wasn't really using sbar.exec at all (only once to spawn the CPU "helper" app) to a configuration that was using sbar.exec everywhere I was using an os.exec or a popen.

The performance improvement was noticeable (really optimistic for this work to get hardened). But I also saw the same flood of "defunct" zombie processes reported in this issue. Seems to point to a bug in the sbar.exec implementation. Maybe something like ignoring SIGCHLD signal of the child process. I'll report back if I play with code more and find something concrete. Have to switch other stuff for now, though.

[FelixKratz]

I think the problem should be fixed by this: https://github.com/FelixKratz/SbarLua/commit/64a6e0d0017cd0bff4926ad4ded6d0c0b458e69d Thanks @shajra for reminding me that I forgot to handle SIGCHLD. I now handle it the same way as in sketchybar.

[shajra]

Okay, I validated that your (much cleaner) approach works. And thanks for the note about the alarm. Setting alarm(0) in my CPU daemon seems to work well to avoid it getting killed.

Everything works swimmingly now, as far as I can tell. The CPU can still climb from all the silly processing with process invocations, but at least it's all done asynchronously. I am curious to see how much of that CPU climb can be reduced with Lua everywhere once your other Lua projects are ready. But for now, this is more than fine.

[FelixKratz]

Maybe the exit code of the command could be supplied as the second argument to the completion handler. That should be fairly simple to do. Then one can choose to ignore it by simply not making the second argument available in the completion handler.

[shajra]

@FelixKratz okay, sounds good. If you remember, ping me if you slip that in without an issue to follow. I have only one place where I hack in an if-then-else wrapper to get an exit code, but I'll take the cleanup if I can get it.

[khaneliman]

I think the problem should be fixed by this: 64a6e0d Thanks @shajra for reminding me that I forgot to handle SIGCHLD. I now handle it the same way as in sketchybar.

Looks like process list is clear of all the defunct processes for me, now. Appreciate the quick fixes for this stuff!

[FelixKratz]

Nice! Do you still see the infinite hotload loop? I think this might a problem when the config changes something in the directory of the config itself... Then the bar will enter a hotload loop. I think I will prohibit this on the sketchybar hotload handling side by introducing some kind of timeout, where a config can not reload arbitrarily often in a certain time period.

EDIT: Implemented on sketchybar master.

[FelixKratz]

Had to swap out the default os.execute implementation because it calls system(1) and gets stuck while trying to retrieve the child exit code because we immediately reap it by ignoring SIGCHLD. https://github.com/FelixKratz/SbarLua/commit/4d429853fcb7db8642db2333d15b84c4c65f0cb9

[khaneliman]

Nice! Do you still see the infinite hotload loop? I think this might a problem when the config changes something in the directory of the config itself... Then the bar will enter a hotload loop. I think I will prohibit this on the sketchybar hotload handling side by introducing some kind of timeout, where a config can not reload arbitrarily often in a certain time period.

EDIT: Implemented on sketchybar master.

I re-enabled hotload after your latest commit, seems to have fixed the loop for me. Thanks