Fix the database locking code in WeBWorK::Utils::CourseIntegrityCheck.

A process should not attempt to unlock the database if it has not first
locked the database.

What is currently happening is that the database integrity checker locks
the database, then unlocks the database, and then when the integrity
checker object is destroyed in clean up it attempts to unlock the
database again (in the DESTROY method).  If there is no lock by any
process when that last unlock attempt is fine.  However, if another
request is sent to the server that also attempts to lock the database
while the first request is still processing, then as soon as this
request unlocks the database, then the second request locks the
database.  Then when the first request attempts to unlock the database
in clean up, it finds the lock now held by the second process and dies
because it can't release the lock now held by the second process.

This now sets a flag when the database integrity checker locks the
database successfully, clears that flag when the lock is successfully
unlocked, and the database is only attempted to be unlocked in clean up
if that flag is still set.  Furthermore, this never dies when attempting
to release a lock if the release attempt fails.  There is no reason for
that.  It just warns in the two cases that a release attempt can fail,
and with the new flag the case of a lock owned by another process should
not occur (unless someone erroneously calls the `unlock_database` method
without first calling the `lock_database` method).

More work needs to be done here.  The course integrity checker is really
not well thought out.

The course database checker and the course directory checker need to be
separated into different modules.  Directory checking and upgrading does
not need a database handle.  If one wanted to check or upgrade directory
structure without checking or upgrading the database, then this would
acquire an unnecessary database handle.  In fact directory checking and
upgrading don't need an object at all.  Those should be exported methods
in another non-object package.

In addition, any there should be checks for exceptions that may be
thrown when the database is checked or upgraded.  There are lots of ways
that can happen in addition to a failed lock attempt (which also really
shouldn't die).

lib/WeBWorK/Utils/CourseIntegrityCheck.pm

@@ -79,8 +79,7 @@ sub confirm { my ($self, @args) = @_; my $sub = $self->{confirm_sub}; return &$s
 
 sub DESTROY {
 	my ($self) = @_;
-	$self->unlock_database;
-	$self->SUPER::DESTROY if $self->can("SUPER::DESTROY");
+	$self->unlock_database if $self->{db_locked};
 	return;
 }
 
@@ -412,29 +411,28 @@ sub updateCourseDirectories {
 # Database utilities -- borrowed from DBUpgrade.pm ??use or modify??? --MEG
 ##############################################################################
 
-sub lock_database {    # lock named 'webwork.dbugrade' times out after 10 seconds
-	my $self          = shift;
-	my $dbh           = $self->dbh;
-	my ($lock_status) = $dbh->selectrow_array("SELECT GET_LOCK('webwork.dbupgrade', 10)");
-	if (not defined $lock_status) {
-		die "Couldn't obtain lock because an error occurred.\n";
-	}
-	if (!$lock_status) {
+# Create a lock named 'webwork.dbugrade' that times out after 10 seconds.
+sub lock_database {
+	my $self = shift;
+	my ($lock_status) = $self->dbh->selectrow_array("SELECT GET_LOCK('webwork.dbupgrade', 10)");
+	if (!defined $lock_status) {
+		die "Couldn't obtain lock because a database error occurred.\n";
+	} elsif (!$lock_status) {
 		die "Timed out while waiting for lock.\n";
 	}
+	$self->{db_locked} = 1;
 	return;
 }
 
 sub unlock_database {
-	my $self          = shift;
-	my $dbh           = $self->dbh;
-	my ($lock_status) = $dbh->selectrow_array("SELECT RELEASE_LOCK('webwork.dbupgrade')");
-	if (not defined $lock_status) {
-		# die "Couldn't release lock because the lock does not exist.\n";
-	} elsif ($lock_status) {
-		return;
+	my $self = shift;
+	my ($lock_status) = $self->dbh->selectrow_array("SELECT RELEASE_LOCK('webwork.dbupgrade')");
+	if ($lock_status) {
+		delete $self->{db_locked};
+	} elsif (defined $lock_status) {
+		warn "Couldn't release lock because the lock is not held by this thread.\n";
 	} else {
-		die "Couldn't release lock because the lock is not held by this thread.\n";
+		warn "Unable to release lock because a database error occurred.\n";
 	}
 	return;
 }