Re: [pacman-dev] [PATCH] repo-add: Remove lock on SIG{TERM, HUP, QUIT, INT, ERR}

On Tue, Oct 11, 2011 at 1:33 PM, Lukas Fleischer wrote:

On Tue, Oct 11, 2011 at 07:03:13PM +0200, Lukas Fleischer wrote:

...

On Tue, Oct 11, 2011 at 11:29:31AM -0500, Dan McGee wrote:

...

On Tue, Oct 11, 2011 at 11:13 AM, Lukas Fleischer wrote:

...

There's no need to keep the lock file if we interrupt repo-{add,remove} using ^C (same applies to other signals). Definite +1 here.

...

In contrast to a clean exit, we do not `rm -rf` the temporary directory here, so that it's still possible to analyze what went wrong. I'm not sure what the right answer is here- I see where you're coming from, but given that we give the user no indication we're leaving a mess behind, I think we should either clean it up or tell the user we did not. Also noticed that we print an error message with TERM even when HUP and QUIT are caught.

I'm not really sure either, but I'd say that showing a message that includes "$tmpdir" might be the best thing to do here.

I also submitted a separate patch that fixes the SIGTERM message issue.

Ugh. It's a bit more complicated, even. Given that we use exit at the very end of trap_exit(), this should already trigger clean_up() automatically. Ahh, true. Odd I didn't think about this before. So this only happens when? If someone hits Ctrl-C in quick succession just right?

The only case where this doesn't work is if we send a signal when clean_up() is already being executed (it's not too hard to reproduce that, actually). So just copying the two lines that unlink the lock file and remove the directory from clean_up() might be the easiest way to go? I'm not sure we can totally eliminate all race conditions. Can we get closer by unhooking all traps at the start of both trap_exit() and clean_up(), and then explicitly calling clean_up() from trap_exit()? The bash manpage is woefully non-descriptive about signal handlers and their reentry or recall procedure- I'm assuming you could theoretically keep sending signals fast enough that signal handlers pile up on the stack and none of them ever complete, so attempting to unhook as soon as possible and not depend on the signal handlers being in place anymore would at least prevent that stack growth.

-Dan