GNOME Bugzilla – Bug 626967
orca's shellscript shouldn't use 'exec' for calling the Python script
Last modified: 2010-09-20 10:57:03 UTC
For launching Orca the user run the shellscript /usr/bin/orca, this shellscript has a cleanup() function that kill all the process Orca leave behind. There is another function that should be lunched just before to exit the script, this is the restoreXmodmap(). Right now there is a workaround to make this work and the cleanup() won't be launched is the user close the app from the GUI (the normal way). This is because the Python code is called from the script using the sentence 'exec'. This open a new process that will replace the current one and all the code after that exec won't be executed. If we remove the token 'exec', when the Python process finish the restoreXmodmap() and cleanup() functions will be called without workarounds or leaving orphan process.
Created attachment 167898 [details] [review] Patch for removing the exec and fix this bug This patch fix the problem described at the summary and make sure the cleanup() function is called, so the process Orca leaves behind are killed.
Comment on attachment 167898 [details] [review] Patch for removing the exec and fix this bug Sweet! That fixes a bug that's in my head as something we needed to fix, namely KP_0 isn't always getting properly restored for the desktop layout. Please commit. And thanks!
Done. I'll close the bug.
Afraid I'm re-opening this. José caught a problem and mentioned it on the list. I can confirm that it only seems to happen with this fix in place. Steps to reproduce: 1. Launch Orca 2. With Orca still running: F2, 'orca --replace', Return Expected results: Orca would be restarted. Actual results: Orca is not restarted. Juanje, would you happen to have time to take a look at this regression? Thanks!
Ouch, sorry for the regression :-/ I'll take care of it. Actually, it must be something wrong with the '--replace' flag because it causes to me a lot of problems debugging the changes for the bug #626968. I'll check what is wrong and I''l fix it. But I'l still thinking it logic should be moved to the Python code and remove the shellscript... I'll try a proof of concept after fix this bug.
Created attachment 167919 [details] [review] Fix for the regression The problem with the '--replace' flag is that the 'cleanup()' function were killing the new Orca process. If we add some sleep time after the function call the new Orca process won't be killed. Putting back the 'exec' token will work also, but if the process die the cleanup and restoreXmodmap functions won't be called. Put a sleep is not the most elegant thing but it work's and let the script exit nicely when it dies. What do you think about it? BTW, could anyone test it? It works in my laptop, but maybe in slower machines it'd need more sleep time...
Hi Juanje, could you take a look in bug #596359? Perhaps it can be closed together. Thanks.
http://git.gnome.org/browse/orca/commit/?id=818542916a2bae6cee92fdbe27f74cbe61a0fde1 Mixed Juanje's proposal and a while to wait for actual instance killing.
(In reply to comment #8) > http://git.gnome.org/browse/orca/commit/?id=818542916a2bae6cee92fdbe27f74cbe61a0fde1 > > Mixed Juanje's proposal and a while to wait for actual instance killing. Ok, as always, copy&paste fail. http://git.gnome.org/browse/orca/commit/?id=8e9c202e487b7a8a8e8d046f2820266a40dc7f71
Created attachment 168056 [details] [review] Fix for the just one process catching problem The current solution commited have some issues. For example, this try to return the pids from the function getOrcaInstances(), but shell functions just can return one numeric value (exit value). In addition the redefinition of the 'IFS' enviroment var was left when the code was moved. In that place it didn't make the job and if the 'ps ...| egrep ...' gets more than one process it won't be returned in the proper way (in single line). I attach a patch which fix those issues and make the code simplier which is always a good thing. If the patch if right for you I'll commit it and close the bug.
(In reply to comment #7) > Hi Juanje, could you take a look in bug #596359? > Perhaps it can be closed together. That would be nice but I not sure if we can do it from the shellscript... I saw the solution at 'accerciser', but I'm not sure if to import gnome library (just to put the name of the process) is something we really want. I'll find another way and If I find it I'll add to the patch. Thanks for the suggest :-)
Review of attachment 168056 [details] [review]: Elegant patch! :) Works for me. Only one issue if you launch fastly two orca --replace (before finishing to load the first orca) we get stalled in the loop. We can fix this issue later and probably also occurs in the commited patch :) Good work!
Created attachment 168106 [details] [review] Fix for the just one process catching problem and something else I made new patch fixing the issue Ale show us. I test myself all the flags, waiting and no waiting for the Orca to finish to be launched and now works better. I also removed the gnome-speecher process to the kill list. After a lot of testing I haven't seen any process (apart the speech-dispatcher which I was told not to add to the list on bug #626968) but the Orca itself on that filter. I guess there are obsolete stuff and that was discussed on the bug #626968. I remove it because I don't see (as Trev teach us) good idea to kill other process and because this way the code can be so much simplier. The solution is sheel agnostic (no bashisms) and should work fine on *Solaris because the comand pkill comes from that OS. If you don't have anything against remove those obsolete process to the list or the code, I'll commit this new patch instead. Ok? Thanks for the reviews ;-)
Comment on attachment 168106 [details] [review] Fix for the just one process catching problem and something else nice shell script engineering :) This is more than is commited now, just push. I think it will tested in the long run :)
Created attachment 168121 [details] [review] Fix for the just one process catching problem and something else It seems like to remove the filter of process wasn't good idea. As I said, it was just in case nothing goes wrong, but it was. GNOME Magnifier is one of the process could have problem with that. I you have it activated and you try to force Orca to quit from the terminal, the magnifier become orphan. I've just fix the patch so we search for the same bunch of process just in case more of those processes have issues as well. I've tested the new patch with the magnifier and everything goes as before, but with the new code. Has anyone something else to point out about this patch? I'll try all the test I've imaged, but sure there are many scenarios I didn't take in mind. Thanks for the feedbak :-)
I've tested every condition and related situation (KP_0 restoration, xmodmap thrashing, etc) I could think of and this works nicely for me. I even verified it works as expected in OpenSolaris. :-) Thanks for the revision! Spelling nit. :-P There are two t's in the word 'pattern.' (And, yes, I DO feel lame for pointing that out.) Since shell scripting ain't my thang, I'd ask that Ale give your patch a look. If he's good with it, I'm good with it. Thanks again!
Review of attachment 168121 [details] [review]: nice patch, push.
Ups... Fixed the typo... :-P Pushed :-) Thanks for your time and reviews.
Hi Juanje, Milton from the mailing list (http://mail.gnome.org/archives/orca-list/2010-August/msg00226.html) reported the following: 1. run orca 2. in gnome-terminal, do orca -v to find out the version of the currently installed version. previously, Orca wasnt killed when we do orca -v, but now it seems that it dies. Varified here. I tracked it down to: commit 818542916a2bae6cee92fdbe27f74cbe61a0fde1 Author: Juanje Ojeda <jojeda@emergya.es> Date: Sun Aug 15 03:16:08 2010 +0100 fix for bug #626967 - Removed exec call and added trap for EXIT signal Could you please look at this? Thanks.
Created attachment 168932 [details] [review] Patch for a better cheking of the orca.py exits Well, the problem here was that the EXIT signal was captured and linked to the funtion cleanup, which kills Orca process but itself. This is ok with options like -q, -f or --replace because they need to cleanup before to exit, but options like -l, -v or -? do their stuff and exit witout killing any previous Orca process. At least, that's the idea. So I figured out a way to keep controling the exit signals but discriminate between them. So this is what I did: * I've removed the trap for the EXIT signal. * I changed the exit value (to 20) on orca.py for the options: -l, -v and -? * I capture the exit value from orca.py and if it's not 20 or the ones that --replace give us, then the cleanup function will be called. I've checked lots of times to launch Orca and then another Orca with each option. Everything seems to work as it should. If anyone see any issue, please, say so.
(In reply to comment #20) > previously, Orca wasnt killed when we do orca -v, but now it seems that it > dies. This turns out to be a problem/bug in orca.die() which Juanje, in fixing the issues here, managed to uncover. As a side note, more and more I am coming to the conclusion that each and any time pylint complains about something and we do one of those "we know what we're doing; pylint is confused" comments, it turns out that we were the ones confused; not pylint. <sighs> Anyhoo, I have opened a new bug (bug 628256) for the newly-uncovered problem. Juanje and I have been working on this and will hopefully have a solution soon. Re-closing this bug as FIXED.
Review of attachment 168932 [details] [review]: Juanje, in the context (brokenness) of Orca's current code, this patch is lovely. Thank you for doing it. But as I just commented, I would much prefer to unbork Orca rather than slap a bandaid on some long-standing breakage. Thus, I am rejecting this patch on the grounds that we should fix Orca. :-)