Menu
▾
▴
1 Attachments
#38 use subprocess32 if available
I have been getting some dead-locks in the library window when doing this:
> rm -rf ~/.local/share/mcomix/library_covers && mcomix -l
Pretty often, some library covers are not re-created, and if I try to open a book, the UI freezes. After investigating, it looks like the fix in process.py for Python bug #1336 is not thread safe.
I could not find a workaround; I tried adding a lock after reading this comment in the bug report, but either this does not work, or I'm still hitting another subprocess related bug, and the dead-locks remain.
After searching for an alternative, I found subprocess32: a backport of the Python 3.2 subprocess module for use on Python versions 2.4 through 2.7. Using it fix my issue.
The attached patch changes process.py to use subprocess32 in place of subprocess when available.
Hey,
I silently merged the patches from the other tickets and closed them, so here you go: thanks! Sorry that it took me a bit longer, I forgot to check my mails. To make things more convenient for you, I added you to SVN access list.
Relating to this patch, I can understand your reasoning, but I would assume that most people wouldn't have subprocess32 installed by default, thus rendering the improvement ineffective.
Maybe it should be considered always spawning processes in the main thread. The inferior threads could wait on a condition that indicates that the main thread has finished spawning the process, and then access the subprocess descriptor. This could be handled similarly to how concurrent.futures works in Python 3. The only problem could be that the main thread still occasionally waits for certain operations to be performed by inferior threads, which would create an ugly dead-lock situation.
Maybe we could also steal relevant code from subprocess32 and implement it with only ctypes.
Thanks for the SVN access!
I have a small fix I'm going to push: https://github.com/benoit-pierre/mcomix/commit/4d33392e8a2ba80295d28ff9d85bb4c3e6126f8c
And then I'd like to push this branch next if you're OK with it: https://github.com/benoit-pierre/mcomix/compare/background-archive-listing
Regarding subprocess handling: both alternative seem like a lot of work when there already is a fix. subprocess32 is just a "pip install" away.
Sure, looks great, so go ahead and commit it.
And, yeah, I realize porting/rewriting already existing code is kind of a pain. Probably not as annoying as getting a working compiler on Windows that can deal with compiling the subprocess32 C modules, though. ;)
Well, I don't know if those specific issues happen on Windows. The code paths are really different, so maybe it's not needed (but there may be other fixes specific to Windows that might be interesting).
I thought you needed a compiler to create a release for Windows, but maybe not (just py2exe?).
I'll go ahead and push the work on background listing / recursive archives.