http-push: remove remote locks on exit signals

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

---

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(-)

index 5b23038..b1f5302 100644
@@ -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

Hi,

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

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

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

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

index 663f18f..b0118d0 100644
@@ -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

Hi,

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

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.

---

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(-)

index 663f18f..b0118d0 100644
@@ -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

Hi,

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

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

Hi,

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

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

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

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

index 5b23038..b44768e 100644
@@ -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