[PATCH] http-push: remove remote locks on exit signals

[PATCH] http-push: remove remote locks on exit signals

[Clemens Buchacher]

If locks are not cleaned up the repository is inaccessible for 10 minutes.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

Hi,

To test this I created a large file, added it, commited, pushed, waited one
second and pressed Ctrl+C. If this method is acceptable for the regression
tests, or if you have a better idea, let me know. I will write up a script.

Regards,
Clemens

---
 http-push.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index 5b23038..b1f5302 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1349,6 +1349,24 @@ static int unlock_remote(struct remote_lock *lock)
 	return rc;
 }
 
+static void remove_locks(void)
+{
+	struct remote_lock *lock = remote->locks;
+	
+	fprintf(stderr, "Removing remote locks...\n");
+	while (lock) {
+		unlock_remote(lock);
+		lock = lock->next;
+	}
+}
+
+static void remove_locks_on_signal(int signo)
+{
+	remove_locks();
+	signal(SIGINT, SIG_DFL);
+	raise(signo);
+}
+
 static void remote_ls(const char *path, int flags,
 		      void (*userFunc)(struct remote_ls_ctx *ls),
 		      void *userData);
@@ -2255,6 +2273,8 @@ int main(int argc, char **argv)
 		goto cleanup;
 	}
 
+	signal(SIGINT, remove_locks_on_signal);
+
 	/* Check whether the remote has server info files */
 	remote->can_update_info_refs = 0;
 	remote->has_info_refs = remote_exists("info/refs");
-- 
1.5.5.1.1.g95a6

Re: [PATCH] http-push: remove remote locks on exit signals

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> diff --git a/http-push.c b/http-push.c
> index 5b23038..b1f5302 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1349,6 +1349,24 @@ static int unlock_remote(struct remote_lock *lock)
>  	return rc;
>  }
>  
> +static void remove_locks(void)
> +{
> +	struct remote_lock *lock = remote->locks;
> +	
> +	fprintf(stderr, "Removing remote locks...\n");
> +	while (lock) {
> +		unlock_remote(lock);
> +		lock = lock->next;
> +	}
> +}
> +
> +static void remove_locks_on_signal(int signo)
> +{
> +	remove_locks();
> +	signal(SIGINT, SIG_DFL);
> +	raise(signo);
> +}
> +

If you caught signo, shouldn't you be resetting that signo not SIGINT?

>  static void remote_ls(const char *path, int flags,
>  		      void (*userFunc)(struct remote_ls_ctx *ls),
>  		      void *userData);
> @@ -2255,6 +2273,8 @@ int main(int argc, char **argv)
>  		goto cleanup;
>  	}
>  
> +	signal(SIGINT, remove_locks_on_signal);
> +

and you may care more than just INT but perhaps HUP and others?


>  	/* Check whether the remote has server info files */
>  	remote->can_update_info_refs = 0;
>  	remote->has_info_refs = remote_exists("info/refs");

Having said that, I do not use http-push myself, so I'll wait for others
who do use it to comment on this; success reports are welcomed, reports on
bad side effects, if any, are even more appreciated.

Re: [PATCH] http-push: remove remote locks on exit signals

[Clemens Buchacher]

Hi,

On Fri, May 23, 2008 at 02:40:41PM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> > +static void remove_locks_on_signal(int signo)
> > +{
> > +	remove_locks();
> > +	signal(SIGINT, SIG_DFL);
> > +	raise(signo);
> > +}
> > +
> 
> If you caught signo, shouldn't you be resetting that signo not SIGINT?

True. I suppose we should fix that in remove_lock_file_on_signal() as well,
then? It does exactly the same thing.

> >  static void remote_ls(const char *path, int flags,
> >  		      void (*userFunc)(struct remote_ls_ctx *ls),
> >  		      void *userData);
> > @@ -2255,6 +2273,8 @@ int main(int argc, char **argv)
> >  		goto cleanup;
> >  	}
> >  
> > +	signal(SIGINT, remove_locks_on_signal);
> > +
> 
> and you may care more than just INT but perhaps HUP and others?

SIGINT was bothering me the most, because I often kill http-push by pressing
Ctrl+C. I also considered adding SIGQUIT, but decided against it because
normally I press Ctrl+\ only if I want the program to quit _right now_ and
Ctrl+C does not work. On the other hand, sending the signal twice will
terminate http-push either way.

I don't know in which situation SIGHUP would come into play.

Regards,
Clemens

[PATCH] Reset the signal being handled

[Clemens Buchacher]

This did not cause any problems, because remove_lock_file_on_signal is
only registered for SIGINT.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 lockfile.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 663f18f..b0118d0 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -24,7 +24,7 @@ static void remove_lock_file(void)
 static void remove_lock_file_on_signal(int signo)
 {
 	remove_lock_file();
-	signal(SIGINT, SIG_DFL);
+	signal(signo, SIG_DFL);
 	raise(signo);
 }
 
-- 
1.5.5.1

Re: [PATCH] Reset the signal being handled

[Johannes Schindelin]

Hi,

On Sun, 25 May 2008, Clemens Buchacher wrote:

> This did not cause any problems, because remove_lock_file_on_signal is
> only registered for SIGINT.

Only from the patch did I understand that you actually meant:

	lockfile: reset the correct signal

	In the function remove_lock_file_on_signal(), the signal handler
	for SIGINT was reset, ignoring the parameter signo.

	This did not pose a problem yet, as remove_lock_file_on_signal()
	was only registered as a SIGINT handler.

Hth,
Dscho

[PATCH] lockfile: reset the correct signal

[Clemens Buchacher]

In the function remove_lock_file_on_signal(), the signal handler
for SIGINT was reset, ignoring the parameter signo.

This did not pose a problem yet, as remove_lock_file_on_signal()
was only registered as a SIGINT handler.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Mon, May 26, 2008 at 10:34:11AM +0100, Johannes Schindelin wrote:
> Only from the patch did I understand that you actually meant:

Thank you for fixing that.

I also realized that using signals like that can cause races. Shouldn't we
use sigaction() instead of signal()?

Clemens
---
 lockfile.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 663f18f..b0118d0 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -24,7 +24,7 @@ static void remove_lock_file(void)
 static void remove_lock_file_on_signal(int signo)
 {
 	remove_lock_file();
-	signal(SIGINT, SIG_DFL);
+	signal(signo, SIG_DFL);
 	raise(signo);
 }
 
-- 
1.5.5.1

Re: [PATCH] lockfile: reset the correct signal

[Johannes Schindelin]

Hi,

On Mon, 26 May 2008, Clemens Buchacher wrote:

> In the function remove_lock_file_on_signal(), the signal handler
> for SIGINT was reset, ignoring the parameter signo.
> 
> This did not pose a problem yet, as remove_lock_file_on_signal()
> was only registered as a SIGINT handler.
> 
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
> 
> On Mon, May 26, 2008 at 10:34:11AM +0100, Johannes Schindelin wrote:
> > Only from the patch did I understand that you actually meant:
> 
> Thank you for fixing that.

Unfortunately, the original patch is already in git.git.

> I also realized that using signals like that can cause races. Shouldn't 
> we use sigaction() instead of signal()?

Dunno.  The man page suggests it, but we have plenty of cases where we use 
signal().  And I think it might be less painful to implement a 
compat-wrapper for the platforms which differ from Linux' interpretation 
of signal().

Ciao,
Dscho

Re: [PATCH] lockfile: reset the correct signal

[Clemens Buchacher]

Hi,

On Mon, May 26, 2008 at 10:36:45PM +0100, Johannes Schindelin wrote:
> > On Mon, May 26, 2008 at 10:34:11AM +0100, Johannes Schindelin wrote:
> > > Only from the patch did I understand that you actually meant:
> > 
> > Thank you for fixing that.
> 
> Unfortunately, the original patch is already in git.git.

I will be more careful with my commit messages in the future.

> > I also realized that using signals like that can cause races. Shouldn't 
> > we use sigaction() instead of signal()?
> 
> Dunno.  The man page suggests it, but we have plenty of cases where we use 
> signal().  And I think it might be less painful to implement a 
> compat-wrapper for the platforms which differ from Linux' interpretation 
> of signal().

On the other hand, sigaction is used already in two places. What do you
think about replacing all those calls to signal/sigaction with something
like this?

void set_signal(int signo, void (*handler)(int), int sa_flags)
{
	struct sigaction sa;

	memset(&sa, 0, sizeof(sa));
	sa.sa_handler = handler;
	sa.sa_flags = sa_flags;
	sigemptyset(&sa.sa_mask);
	sigaction(signo, &sa, NULL);
}

The behavior won't change, except that handlers cannot be interrupted by
their own signal.

Clemens

[PATCH] http-push: remove remote locks on exit signals

[Clemens Buchacher]

If locks are not cleaned up the repository is inaccessible for 10 minutes.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 http-push.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index 5b23038..b44768e 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1349,6 +1349,24 @@ static int unlock_remote(struct remote_lock *lock)
 	return rc;
 }
 
+static void remove_locks(void)
+{
+	struct remote_lock *lock = remote->locks;
+	
+	fprintf(stderr, "Removing remote locks...\n");
+	while (lock) {
+		unlock_remote(lock);
+		lock = lock->next;
+	}
+}
+
+static void remove_locks_on_signal(int signo)
+{
+	remove_locks();
+	signal(signo, SIG_DFL);
+	raise(signo);
+}
+
 static void remote_ls(const char *path, int flags,
 		      void (*userFunc)(struct remote_ls_ctx *ls),
 		      void *userData);
@@ -2255,6 +2273,10 @@ int main(int argc, char **argv)
 		goto cleanup;
 	}
 
+	signal(SIGINT, remove_locks_on_signal);
+	signal(SIGHUP, remove_locks_on_signal);
+	signal(SIGQUIT, remove_locks_on_signal);
+
 	/* Check whether the remote has server info files */
 	remote->can_update_info_refs = 0;
 	remote->has_info_refs = remote_exists("info/refs");
-- 
1.5.5.1