Add-on is keeping skin files open which prevent skin updates

[tomer953]

as I explained in the forum thread:
http://forum.kodi.tv/showthread.php?tid=257967&pid=2231316#pid2231316

The process of pulsar.exe is using Textures.xbt file, which causing kodi to crash when I push skin updates to my users.

[scakemyer]

Just ran into this while upgrading Kodi. It's quite a nasty bug.

[tomer953]

Also, I forgot to mention, that from My experience, it's not happening when the skin is hosted by the official repository, only from private repo's.

[tomer953]

@scakemyer , sorry for bumping it...
but other skinners are keep asking me if this already has been reported\solved..
so I wonder if you have spare time to look into it.

many thanks for your hard work.

[scakemyer]

I haven't forgotten about this, I just currently don't have any clue what is causing this. Could you provide a more detailed debug log? (please no pastebin or forum link, really tired of having to solve CloudFlare captchas)

[tomer953]

Actually, my skin is already in the official repository, which for some weired reason is not affected with this problem.
However, I have a small log from the days my skin was in private repo:
http://xbmclogs.com/pucp0vqvb

I had this issue every few days when I pushed new updates to my repo, I also searched for some more details in the log - but none. even in debug mode.
one day, while updating failed, and I had the "black screen" issue, I went to my skin dir, to see what is going on there - then I saw that almost all the file deleted (like they should, when updating), except for the "Textures.xbt" file in the "media" folder. when I tried to "shift+delete" it, with windows explorer, I recieved a msg that "pulsar.exe" is using this file, and it cannot be removed.
that's all the details I have..

If you have some clue about it, and you need some testing, I have friends with private skins which can help us.

[ShlomiD83]

sorry for bumping this, but i have the same problem with Xonfluence skin..

[guidosarducci]

This bug is definitely nasty, and aggravated me to point I just had to kill it. The core issue is that the quasar binary inherits all of Kodi's open files, including things like skin fonts and graphics, which means they can't be deleted and replaced as part of an update. An even bigger PITA is that no one solution will work on both Android (my platform) and Windows (not my platform), but I think I have an approach for each.

Before I PR this, I'd appreciate if some other guinea pigs users could try out the updated quasar plugin I attached, especially on Windows, and provide feedback.

You can confirm the issue with inheritied files yourself using 'lsof' on Unix (Linux/Android/etc) or 'handle.exe' on Windows (Handle v4.0). Try doing this with your old quasar installation and then my updated one to see the difference.

plugin.video.quasar-fix-close-quasar-fds.zip

[scakemyer]

Can you explain how you're solving this and pushing your code to your branch even if not doing a PR yet?

[guidosarducci]

... pushing your code to your branch even if not doing a PR yet?

Could you explain this part of your question? It's not clear what you're asking. Do you prefer I just PR the changes and move discussion there?

Can you explain how you're solving this ...

Sure. First some background information:
PEP 0446
Python Doc: Subprocess Management
W32 CreateProcess function
W32 SetHandleInformation function

All changes are localzed to the resources/site-packages/quasar/daemon.py file, which manages spawning the child process for the quasar binary. The changes are clear and isolated, so please review those first.

Next, let me expand on how the quasar binary is inheriting open files, with a few key points:

• Kodi's version of Python has new file descriptors created to be inheritable
• by default child processes from a call to subprocess.popen() will inherit permitted files
• the quasar binary child process has its stdout redirected via a pipe (important for later)

The other important thing to note is from the Python docs on subprocess.popen() call paramters:

If close_fds is true, all file descriptors except 0, 1 and 2 will be closed before the child process is executed. (Unix only). Or, on Windows, if close_fds is true then no handles will be inherited by the child process. Note that on Windows, you cannot set close_fds to true and also redirect the standard handles by setting stdin, stdout or stderr.

And here is where the solution gets more complicated. On Unix (Linux/Android/etc) I just add the parameter close_fds = True to the subprocess.popen() call. But this won't work on Windows because the plugin needs to use a pipe to read the quasar binary stdout. To work around this limitation on Windows, I instead call a new clear_fd_inherit_flags() function before the normal subprocess.popen() call. This function iterates over open regular files/directories and marks them as non-inheritable, so that the quasar child process will not hold them open and interfere with addon updates.

Now, I've reproduced the problem/solution in Python on an old Windows box, but I don't run Kodi there, which is why I asked someone to help test it on Windows.

Anyone?

[ShlomiD83]

your fix isn't working on windows.
i just need to copy the files from your zip and try to update the skin?

[teamThevibe]

anyone test the fix ?
i want to update my skin ... :)

[ShlomiD83]

1 comment above you, not working on windows.

[metate]

@guidosarducci It won't work on windows because file handles are not the same as posix fds. The handle for textures.xbt can be above 1000. See:

 ------------------------------------------------------------------------------
quasar.exe pid: 14316 DESKTOP-PJ5OMUK\metate
   10: File  (---)   C:\Users\metate\AppData\Roaming\Kodi\userdata\addon_data\plugin.video.quasar\bin\windows_x64
  1F4: File  (---)   C:\Windows\System32\en-US\mswsock.dll.mui
  6F0: File  (---)   C:\Users\metate\AppData\Roaming\Kodi\addons\skin.phenomenal\media\Textures.xbt
------------------------------------------------------------------------------

I would suggest setting close_fds=True on all platforms (confirmed working). This will require removing stdout and stderr redirects on windows so IMHO the best way to handle it is to replace the logging code to use the quasar RPC mechanism instead (preferably using batch logging to avoid overhead).

Edit: the code already supports RPC for logging. We don't really need to redirect stdout/stderr. The fix should be fairly easy.

Nice catch @guidosarducci

[tomer953]

Sorry for being away, thank you all, great work.

@scakemyer ... thoughts?

[guidosarducci]

Was away for a little bit so catching up...

@metate Thanks for trying my suggestion of using handle.exe. When I hadn't seen a response from anyone I broke down and built a Kodi Windows VM, and you confirmed what I first saw as well.

@scakemyer The root problem is a messy one, and a Kodi bug AFAIC. Kodi leaks some files to child processes, and prevents others from being inherited. Worse, the files that are leaked depend on the platform and runtime used. This stuff should properly be fixed in Kodi itself.

That said, I've updated my Windows workaround to use the pure MS CRT, and confirmed it works on Kodi Windows. I'll do a PR shortly, and folks can try it from there.

[ShlomiD83]

confirmed working on windows, thnx @guidosarducci .

[scakemyer]

Awesome work here @guidosarducci, sorry I was also away the past few weeks.

[scakemyer]

Should be fixed by #469 thanks to @guidosarducci