Closed Bug 252450 Opened 19 years ago Closed 19 years ago
Tables unlocked too late in several error handlers using Put
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0) Build Identifier: There are places in editcomponents.cgi where SendSQL("UNLOCK TABLES") happens after PutTrailer(). This causes the footer to crash with "Table 'user_group_map' was not locked with LOCK TABLES [for Statement "SELECT 1 FROM user_group_map WHERE user_id=? AND isbless=1"] at Bugzilla/User.pm line 323". There are places where the order is all right, and editproducts.cgi does it right at all places. Reproducible: Always Steps to Reproduce: 1. Try to change the case of a letter in a component name. Actual Results: Expected error message "Sorry, component name 'xyZ' is already in use" is displayed plus "Table 'user_group_map' was not locked with LOCK TABLES [for Statement "SELECT 1 FROM user_group_map WHERE user_id=? AND isbless=1"] at Bugzilla/User.pm line 323". Expected Results: Bugzilla should show the expected error message and the correct footer. I'll post a patch as soon as I can use cvs next time.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
Just noting that this bug is probably a dupe of bug#64192, but the patch on that one got bogged down in other issues, which I never finished resolving :( The patch for editcomponents templatisation (bug#190220) also fixes the places where this still happens, I think.
Ok, let's move over to bug 64192. I'll ask for blocking2.18 there because we have it here. Bug 190220 is targeted for 2.20, so I suggest going for a fix even if it turns out to be redundant. *** This bug has been marked as a duplicate of 64192 ***
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → DUPLICATE
Assignee: justdave → bugreport
Status: REOPENED → ASSIGNED
*** Bug 64192 has been marked as a duplicate of this bug. ***
(In reply to comment #3) > Created an attachment (id=154098) > centralize the unlock This works very well for me.
Same change as the previous patch, but to all instances of PutTrailer().
Morphing the bug accordingly.
Summary: Tables unlocked too late in some of editcomponents.cgi's error handlers → Tables unlocked too late in several error handlers using PutTrailer()
SendSQL() starts with an uppercase 'S', but it is depreciated in favour of: $dbh->do("UNLOCK TABLES"); in the files which have been updated to use $dbh. Is it OK (across future different types of DB) to UNLOCK tables when none have been locked?
sendSQL -> SendSQL. The database question needs to be addressed elsewhere, I'd say.
Attachment #154102 - Attachment is obsolete: true
As talked about on irc.
This error is generated inside Bugzilla::User->queries -- it's the only place we emit that query. Still looking.
Apparently ->queries() is being called very early on my setup -- before the header shows up -- and that's why I don't trigger the error explicitly. I'm trying to figure out why. Aham, queries is being loaded when PutHeader() is called, more precisely, when it does a $::template->process(). Why? Hmmm. Carp::longmess is my friend. ARGH. Of course. Because of site-navigation's collection of links! That's why we're hitting user.queries so early (before actually printing the actual header, even), and then getting it cached so we don't actually call it ever inside editcomponents.cgi and friends. And I *bet* Marc's site doesn't use the standard header, or if so, that it doesn't include useful-links. Now that I can reproduce this, it's probably easier to find out if this patch actually fixes the bug. :-)
Comment on attachment 154108 [details] [diff] [review] centralize unlocking in all PutTrailer-cgis -- syntax corrected This is verifiably safe in this specific situation. (I am a bit uneasy about UNLOCK TABLES in general because when these errors show up it's often because we're throwing an error in the middle of a database write and we can end up with inconsistent state. Of course, we're already inconsistent, but the fact that we raise a real hard error means someone is likely to report it as a bug and we're going to look into it.) This patch centralizes unlocks into a single location and makes it forceably consistent. I'm for it, and it should be low-risk enough for 2.18rc2.
Attachment #154108 - Flags: review? → review+
Requesting approval for trunk and branch. This isn't triggered in the default installation AFAICS but is still an issue for customizers (as it's hidden by sheer casuality right now).
ask for approval again after you get it reviewed :)
(In reply to comment #16) > ask for approval again after you get it reviewed :) I'm confused now: didn't Christian r+ my patch at time of comment 15? Requesting approval again; please tell me if I'm misunderstood.
Status: NEW → ASSIGNED
Sorry, you are correct. There are non-patch attachments after the patch, which confused me.
checked in on both branches
Status: ASSIGNED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.