Closed Bug 189388 Opened 19 years ago Closed 17 years ago

Add run-time check for nested locks

Categories

(Bugzilla :: Administration, task, P4)

2.17.1

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: bugreport, Assigned: bugreport)

References

Details

(Whiteboard: [blocker will fix])

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.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
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?
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.
Severity: major → enhancement
Priority: P2 → P4
Summary: editcomponents.cgi can deadlock (maybe others) → Add run-time check for nested locks
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.
I think the Postgres patch does this, or could really easily....
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)
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
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. :-)
Depends on: bz-postgres
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.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
In fact, this is done once we start using the bz_lock_tables function, which
happens in bug 280503.
Depends on: 280503
No longer depends on: bz-postgres
Whiteboard: [blocker will fix]
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
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.
Bug 280503 covers it.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → WORKSFORME
clearing target of DUPLICATE/WONTFIX/INVALID/WORKSFORME so they'll show up as
untriaged if they get reopened.
Target Milestone: Bugzilla 2.20 → ---
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.