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

RESOLVED FIXED in Bugzilla 2.18

Status

()

P2
normal
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: Wurblzap, Assigned: bugreport)

Tracking

unspecified
Bugzilla 2.18
Bug Flags:
approval +
approval2.18 +
blocking2.18 +

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Assignee)

Updated

14 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking2.18?
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
Flags: blocking2.18? → blocking2.18+

Comment 1

14 years ago
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.
(Reporter)

Comment 2

14 years ago
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
Last Resolved: 14 years ago
Resolution: --- → DUPLICATE
(Assignee)

Updated

14 years ago
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 3

14 years ago
Created attachment 154098 [details] [diff] [review]
centralize the unlock
(Assignee)

Updated

14 years ago
Assignee: justdave → bugreport
Status: REOPENED → ASSIGNED
(Assignee)

Comment 4

14 years ago
*** Bug 64192 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

14 years ago
Attachment #154098 - Flags: review?
(Reporter)

Comment 5

14 years ago
(In reply to comment #3)
> Created an attachment (id=154098)
> centralize the unlock

This works very well for me.
(Reporter)

Comment 6

14 years ago
Created attachment 154102 [details] [diff] [review]
centralize unlocking in all PutTrailer-cgis

Same change as the previous patch, but to all instances of PutTrailer().
(Reporter)

Comment 7

14 years ago
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()

Comment 8

14 years ago
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?
(Reporter)

Comment 9

14 years ago
Created attachment 154108 [details] [diff] [review]
centralize unlocking in all PutTrailer-cgis -- syntax corrected

sendSQL -> SendSQL.
The database question needs to be addressed elsewhere, I'd say.
(Reporter)

Updated

14 years ago
Attachment #154102 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
Attachment #154108 - Flags: review?

Updated

14 years ago
Attachment #154098 - Flags: review?
(Reporter)

Comment 10

14 years ago
Created attachment 154115 [details]
Screenshot of footer

As talked about on irc.
(Reporter)

Comment 11

14 years ago
Created attachment 154116 [details]
Full error message

Comment 12

14 years ago
This error is generated inside Bugzilla::User->queries -- it's the only place we
emit that query. Still looking.

Comment 13

14 years ago
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 14

14 years ago
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+

Comment 15

14 years ago
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?

Updated

14 years ago
Assignee: bugreport → marcschum
Status: ASSIGNED → NEW

Updated

14 years ago
Attachment #154098 - Attachment is obsolete: true
ask for approval again after you get it reviewed :)
Flags: approval?
Flags: approval2.18?
(Reporter)

Comment 17

14 years ago
(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
(Reporter)

Updated

14 years ago
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)

Comment 19

14 years ago
Created attachment 154422 [details] [diff] [review]
Same patch as 154108 ported to 2_19
Assignee: marcschum → bugreport
Attachment #154115 - Attachment is obsolete: true
Attachment #154116 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #154422 - Flags: review?

Updated

14 years ago
Attachment #154422 - Flags: review? → review+
(Assignee)

Comment 20

14 years ago
checked in on both branches
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago14 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.