{"thread":{"id":"13618","subject":"[PATCH] http-push: remove remote locks on exit signals","startedAt":"2008-05-22T19:55:46Z","lastAt":"2008-05-27T07:49:03Z","messageCount":9,"participants":["Clemens Buchacher","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"77491","messageId":"20080522195546.GA29911@localhost","threadId":"13618","inReplyTo":null,"subject":"[PATCH] http-push: remove remote locks on exit signals","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-05-22T19:55:46Z","receivedAt":"2008-05-22T19:55:46Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If locks are not cleaned up the repository is inaccessible for 10 minutes.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nHi,\n\nTo test this I created a large file, added it, commited, pushed, waited one\nsecond and pressed Ctrl+C. If this method is acceptable for the regression\ntests, or if you have a better idea, let me know. I will write up a script.\n\nRegards,\nClemens\n\n---\n http-push.c |   20 ++++++++++++++++++++\n 1 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 5b23038..b1f5302 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1349,6 +1349,24 @@ static int unlock_remote(struct remote_lock *lock)\n \treturn rc;\n }\n \n+static void remove_locks(void)\n+{\n+\tstruct remote_lock *lock = remote->locks;\n+\t\n+\tfprintf(stderr, \"Removing remote locks...\\n\");\n+\twhile (lock) {\n+\t\tunlock_remote(lock);\n+\t\tlock = lock->next;\n+\t}\n+}\n+\n+static void remove_locks_on_signal(int signo)\n+{\n+\tremove_locks();\n+\tsignal(SIGINT, SIG_DFL);\n+\traise(signo);\n+}\n+\n static void remote_ls(const char *path, int flags,\n \t\t      void (*userFunc)(struct remote_ls_ctx *ls),\n \t\t      void *userData);\n@@ -2255,6 +2273,8 @@ int main(int argc, char **argv)\n \t\tgoto cleanup;\n \t}\n \n+\tsignal(SIGINT, remove_locks_on_signal);\n+\n \t/* Check whether the remote has server info files */\n \tremote->can_update_info_refs = 0;\n \tremote->has_info_refs = remote_exists(\"info/refs\");\n-- \n1.5.5.1.1.g95a6\n"},{"id":"77603","messageId":"7vod6wr95y.fsf@gitster.siamese.dyndns.org","threadId":"13618","inReplyTo":"20080522195546.GA29911@localhost","subject":"Re: [PATCH] http-push: remove remote locks on exit signals","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2008-05-23T21:40:41Z","receivedAt":"2008-05-23T21:40:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> diff --git a/http-push.c b/http-push.c\n> index 5b23038..b1f5302 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -1349,6 +1349,24 @@ static int unlock_remote(struct remote_lock *lock)\n>  \treturn rc;\n>  }\n>  \n> +static void remove_locks(void)\n> +{\n> +\tstruct remote_lock *lock = remote->locks;\n> +\t\n> +\tfprintf(stderr, \"Removing remote locks...\\n\");\n> +\twhile (lock) {\n> +\t\tunlock_remote(lock);\n> +\t\tlock = lock->next;\n> +\t}\n> +}\n> +\n> +static void remove_locks_on_signal(int signo)\n> +{\n> +\tremove_locks();\n> +\tsignal(SIGINT, SIG_DFL);\n> +\traise(signo);\n> +}\n> +\n\nIf you caught signo, shouldn't you be resetting that signo not SIGINT?\n\n>  static void remote_ls(const char *path, int flags,\n>  \t\t      void (*userFunc)(struct remote_ls_ctx *ls),\n>  \t\t      void *userData);\n> @@ -2255,6 +2273,8 @@ int main(int argc, char **argv)\n>  \t\tgoto cleanup;\n>  \t}\n>  \n> +\tsignal(SIGINT, remove_locks_on_signal);\n> +\n\nand you may care more than just INT but perhaps HUP and others?\n\n\n>  \t/* Check whether the remote has server info files */\n>  \tremote->can_update_info_refs = 0;\n>  \tremote->has_info_refs = remote_exists(\"info/refs\");\n\nHaving said that, I do not use http-push myself, so I'll wait for others\nwho do use it to comment on this; success reports are welcomed, reports on\nbad side effects, if any, are even more appreciated.\n"},{"id":"77604","messageId":"20080523221723.GA4366@localhost","threadId":"13618","inReplyTo":"7vod6wr95y.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] http-push: remove remote locks on exit signals","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-05-23T22:17:23Z","receivedAt":"2008-05-23T22:17:23Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Hi,\n\nOn Fri, May 23, 2008 at 02:40:41PM -0700, Junio C Hamano wrote:\n> Clemens Buchacher <drizzd@aon.at> writes:\n> > +static void remove_locks_on_signal(int signo)\n> > +{\n> > +\tremove_locks();\n> > +\tsignal(SIGINT, SIG_DFL);\n> > +\traise(signo);\n> > +}\n> > +\n> \n> If you caught signo, shouldn't you be resetting that signo not SIGINT?\n\nTrue. I suppose we should fix that in remove_lock_file_on_signal() as well,\nthen? It does exactly the same thing.\n\n> >  static void remote_ls(const char *path, int flags,\n> >  \t\t      void (*userFunc)(struct remote_ls_ctx *ls),\n> >  \t\t      void *userData);\n> > @@ -2255,6 +2273,8 @@ int main(int argc, char **argv)\n> >  \t\tgoto cleanup;\n> >  \t}\n> >  \n> > +\tsignal(SIGINT, remove_locks_on_signal);\n> > +\n> \n> and you may care more than just INT but perhaps HUP and others?\n\nSIGINT was bothering me the most, because I often kill http-push by pressing\nCtrl+C. I also considered adding SIGQUIT, but decided against it because\nnormally I press Ctrl+\\ only if I want the program to quit _right now_ and\nCtrl+C does not work. On the other hand, sending the signal twice will\nterminate http-push either way.\n\nI don't know in which situation SIGHUP would come into play.\n\nRegards,\nClemens\n"},{"id":"77713","messageId":"20080525182650.GA17806@localhost","threadId":"13618","inReplyTo":"20080523221723.GA4366@localhost","subject":"[PATCH] Reset the signal being handled","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-05-25T18:26:50Z","receivedAt":"2008-05-25T18:26:50Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"This did not cause any problems, because remove_lock_file_on_signal is\nonly registered for SIGINT.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n lockfile.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 663f18f..b0118d0 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -24,7 +24,7 @@ static void remove_lock_file(void)\n static void remove_lock_file_on_signal(int signo)\n {\n \tremove_lock_file();\n-\tsignal(SIGINT, SIG_DFL);\n+\tsignal(signo, SIG_DFL);\n \traise(signo);\n }\n \n-- \n1.5.5.1\n"},{"id":"77714","messageId":"20080525182744.GB17806@localhost","threadId":"13618","inReplyTo":"20080523221723.GA4366@localhost","subject":"[PATCH] http-push: remove remote locks on exit signals","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-05-25T18:27:44Z","receivedAt":"2008-05-25T18:27:44Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If locks are not cleaned up the repository is inaccessible for 10 minutes.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n http-push.c |   22 ++++++++++++++++++++++\n 1 files changed, 22 insertions(+), 0 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 5b23038..b44768e 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1349,6 +1349,24 @@ static int unlock_remote(struct remote_lock *lock)\n \treturn rc;\n }\n \n+static void remove_locks(void)\n+{\n+\tstruct remote_lock *lock = remote->locks;\n+\t\n+\tfprintf(stderr, \"Removing remote locks...\\n\");\n+\twhile (lock) {\n+\t\tunlock_remote(lock);\n+\t\tlock = lock->next;\n+\t}\n+}\n+\n+static void remove_locks_on_signal(int signo)\n+{\n+\tremove_locks();\n+\tsignal(signo, SIG_DFL);\n+\traise(signo);\n+}\n+\n static void remote_ls(const char *path, int flags,\n \t\t      void (*userFunc)(struct remote_ls_ctx *ls),\n \t\t      void *userData);\n@@ -2255,6 +2273,10 @@ int main(int argc, char **argv)\n \t\tgoto cleanup;\n \t}\n \n+\tsignal(SIGINT, remove_locks_on_signal);\n+\tsignal(SIGHUP, remove_locks_on_signal);\n+\tsignal(SIGQUIT, remove_locks_on_signal);\n+\n \t/* Check whether the remote has server info files */\n \tremote->can_update_info_refs = 0;\n \tremote->has_info_refs = remote_exists(\"info/refs\");\n-- \n1.5.5.1\n"},{"id":"77753","messageId":"alpine.DEB.1.00.0805261031480.30431@racer","threadId":"13618","inReplyTo":"20080525182650.GA17806@localhost","subject":"Re: [PATCH] Reset the signal being handled","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-26T09:34:11Z","receivedAt":"2008-05-26T09:34:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 25 May 2008, Clemens Buchacher wrote:\n\n> This did not cause any problems, because remove_lock_file_on_signal is\n> only registered for SIGINT.\n\nOnly from the patch did I understand that you actually meant:\n\n\tlockfile: reset the correct signal\n\n\tIn the function remove_lock_file_on_signal(), the signal handler\n\tfor SIGINT was reset, ignoring the parameter signo.\n\n\tThis did not pose a problem yet, as remove_lock_file_on_signal()\n\twas only registered as a SIGINT handler.\n\nHth,\nDscho\n"},{"id":"77791","messageId":"20080526193513.GA9978@localhost","threadId":"13618","inReplyTo":"alpine.DEB.1.00.0805261031480.30431@racer","subject":"[PATCH] lockfile: reset the correct signal","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-05-26T19:35:13Z","receivedAt":"2008-05-26T19:35:13Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"In the function remove_lock_file_on_signal(), the signal handler\nfor SIGINT was reset, ignoring the parameter signo.\n\nThis did not pose a problem yet, as remove_lock_file_on_signal()\nwas only registered as a SIGINT handler.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nOn Mon, May 26, 2008 at 10:34:11AM +0100, Johannes Schindelin wrote:\n> Only from the patch did I understand that you actually meant:\n\nThank you for fixing that.\n\nI also realized that using signals like that can cause races. Shouldn't we\nuse sigaction() instead of signal()?\n\nClemens\n---\n lockfile.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 663f18f..b0118d0 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -24,7 +24,7 @@ static void remove_lock_file(void)\n static void remove_lock_file_on_signal(int signo)\n {\n \tremove_lock_file();\n-\tsignal(SIGINT, SIG_DFL);\n+\tsignal(signo, SIG_DFL);\n \traise(signo);\n }\n \n-- \n1.5.5.1\n"},{"id":"77802","messageId":"alpine.DEB.1.00.0805262234210.30431@racer","threadId":"13618","inReplyTo":"20080526193513.GA9978@localhost","subject":"Re: [PATCH] lockfile: reset the correct signal","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-26T21:36:45Z","receivedAt":"2008-05-26T21:36:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 26 May 2008, Clemens Buchacher wrote:\n\n> In the function remove_lock_file_on_signal(), the signal handler\n> for SIGINT was reset, ignoring the parameter signo.\n> \n> This did not pose a problem yet, as remove_lock_file_on_signal()\n> was only registered as a SIGINT handler.\n> \n> Signed-off-by: Clemens Buchacher <drizzd@aon.at>\n> ---\n> \n> On Mon, May 26, 2008 at 10:34:11AM +0100, Johannes Schindelin wrote:\n> > Only from the patch did I understand that you actually meant:\n> \n> Thank you for fixing that.\n\nUnfortunately, the original patch is already in git.git.\n\n> I also realized that using signals like that can cause races. Shouldn't \n> we use sigaction() instead of signal()?\n\nDunno.  The man page suggests it, but we have plenty of cases where we use \nsignal().  And I think it might be less painful to implement a \ncompat-wrapper for the platforms which differ from Linux' interpretation \nof signal().\n\nCiao,\nDscho\n"},{"id":"77827","messageId":"20080527074903.GA2403@localhost","threadId":"13618","inReplyTo":"alpine.DEB.1.00.0805262234210.30431@racer","subject":"Re: [PATCH] lockfile: reset the correct signal","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-05-27T07:49:03Z","receivedAt":"2008-05-27T07:49:03Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Hi,\n\nOn Mon, May 26, 2008 at 10:36:45PM +0100, Johannes Schindelin wrote:\n> > On Mon, May 26, 2008 at 10:34:11AM +0100, Johannes Schindelin wrote:\n> > > Only from the patch did I understand that you actually meant:\n> > \n> > Thank you for fixing that.\n> \n> Unfortunately, the original patch is already in git.git.\n\nI will be more careful with my commit messages in the future.\n\n> > I also realized that using signals like that can cause races. Shouldn't \n> > we use sigaction() instead of signal()?\n> \n> Dunno.  The man page suggests it, but we have plenty of cases where we use \n> signal().  And I think it might be less painful to implement a \n> compat-wrapper for the platforms which differ from Linux' interpretation \n> of signal().\n\nOn the other hand, sigaction is used already in two places. What do you\nthink about replacing all those calls to signal/sigaction with something\nlike this?\n\nvoid set_signal(int signo, void (*handler)(int), int sa_flags)\n{\n\tstruct sigaction sa;\n\n\tmemset(&sa, 0, sizeof(sa));\n\tsa.sa_handler = handler;\n\tsa.sa_flags = sa_flags;\n\tsigemptyset(&sa.sa_mask);\n\tsigaction(signo, &sa, NULL);\n}\n\nThe behavior won't change, except that handlers cannot be interrupted by\ntheir own signal.\n\nClemens\n"}]}