189388 - Add run-time check for nested locks

Status: RESOLVED WORKSFORME

(Bugzilla :: Administration, task, P4)

Description

[Joel Peshkin]

When changing bug owners, editcomponents.cgi may call functions withing a
section where the databases are locked that, in turn, try to lock the tables.
That can cause a deadlock.

This should not be permitted and needs to be scrubbed.  This needs a lock
counter to turn attempts to lock while already locked into exceptions.

Comment 1

[Bradley Baetz (:bbaetz)]

No generic functions should ever LOCK tables. Not only because of this, but
because the unlock would then unlock all of the locks.
Which particular functions have locks?

Comment 2

[Joel Peshkin]

It looks like the cause for this was already fixed since I pulled the code for
my live site.  It used to be that DBnameToId was calling derivegroups and
populating the user data structure.  The problem was that it was then being
called from within a locked section in editcomponents.cgi

Even that was not enough to surface the problem because mysql treats the attempt
to lock while already locked as a replacement of one set of locks with another.
 So, this required the code to get into a state where it cannot get the new lock
and, therefore, held onto the old lock in order to cause the deadlock.

The only scenario I can think of that would let that deadlock is if ...

editcomponents locks set A and calls something that tries to lock set B
meanwhile, another thread that already has something in set B locked is trying
to lock something in set A.

It is kind of silly that MySQL doesn't consider a LOCK attempt when already
locked to be a run-time error.  What we should do is store caller() when
Bugzilla::DB is asked to LOCK and clear it when it is asked to UNLOCK.  If it is
already set when we are asked to LOCK, throw an exception [dumping caller()
andthe prior caller()]

Incidentally, we really should find away to dump caller() when there is an SQL
error as well.

Comment 3

[Bradley Baetz (:bbaetz)]

Well, I dislike derivegroups, as you know :)

postgres tests for deadlocks, and mysql claims to avoid them, although I don't
know if that covers LOCK over multiple statements.

We don't want to check them, we want to avoid haing a situation where we can
ever get into that case.

If we had transactions, then we could just recalculate thr groups, and if two
processes happened to do that then thy'd have the same answer, so it doesn't matter.

Re dumping for sql errors, see Exception::Class::DBI, + my exceptions thread
from sep/oct. We don't want caller() though, but there is a dbi property we can
use to get the equiv result.

Comment 4

[Dave Miller [:justdave]]

I think the Postgres patch does this, or could really easily....

Comment 5

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 6

[Max Kanat-Alexander]

Yeah, in fact, LockTables() could check if the tables are already locked, if
there's a way to do that... That would avoid deadlocks in all situations. It
could also throw a warning when it happens, so that we could get an idea of when
it's happening.

And, of course, it would be easy to remove the inappripriate LOCK TABLES
statements, since dkl will know where all of them are. :-)

Comment 7

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 8

[Max Kanat-Alexander]

In fact, this is done once we start using the bz_lock_tables function, which
happens in bug 280503.

Comment 9

[Tomas Kopal]

As bug 280503 is in, I think this is now fixed. Please, take a look at the
solution implemented and if you are happy with it, close this bug.

Comment 10

[Joel Peshkin]

Bug 280503 covers it.

Comment 11

[Dave Miller [:justdave]]

clearing target of DUPLICATE/WONTFIX/INVALID/WORKSFORME so they'll show up as
untriaged if they get reopened.