Closed Bug 252450 Opened 19 years ago Closed 19 years ago

Tables unlocked too late in several error handlers using PutTrailer()

Categories

(Bugzilla :: Administration, task, P2)

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: Wurblzap, Assigned: bugreport)

References

Details

Attachments

(2 files, 4 obsolete files)

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
Flags: blocking2.18?
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
Flags: blocking2.18? → blocking2.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
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attached patch centralize the unlock (obsolete) — Splinter Review
Assignee: justdave → bugreport
Status: REOPENED → ASSIGNED
*** Bug 64192 has been marked as a duplicate of this bug. ***
Attachment #154098 - Flags: review?
(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
Attachment #154108 - Flags: review?
Attachment #154098 - Flags: review?
Attached image Screenshot of footer (obsolete) —
As talked about on irc.
Attached file Full error message (obsolete) —
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).
Flags: approval?
Flags: approval2.18?
Assignee: bugreport → marcschum
Status: ASSIGNED → NEW
Attachment #154098 - Attachment is obsolete: true
ask for approval again after you get it reviewed :)
Flags: approval?
Flags: approval2.18?
(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
Flags: approval?
Flags: approval2.18?
Sorry, you are correct.  There are non-patch attachments after the patch, which
confused me.
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
Assignee: marcschum → bugreport
Attachment #154115 - Attachment is obsolete: true
Attachment #154116 - Attachment is obsolete: true
Attachment #154422 - Flags: review?
Attachment #154422 - Flags: review? → review+
checked in on both branches
Status: ASSIGNED → RESOLVED
Closed: 19 years ago19 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.