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

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P2)

Description

[Marc Schumann [:Wurblzap]]

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.

Comment 1

[GavinS]

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.

Comment 2

[Marc Schumann [:Wurblzap]]

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 ***

Comment 3

[Joel Peshkin]

Attachment: centralize the unlock

Comment 4

[Joel Peshkin]

*** Bug 64192 has been marked as a duplicate of this bug. ***

Comment 5

[Marc Schumann [:Wurblzap]]

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

This works very well for me.

Comment 6

[Marc Schumann [:Wurblzap]]

Attachment: centralize unlocking in all PutTrailer-cgis

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

Comment 7

[Marc Schumann [:Wurblzap]]

Morphing the bug accordingly.

Comment 8

[GavinS]

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?

Comment 9

[Marc Schumann [:Wurblzap]]

Attachment: centralize unlocking in all PutTrailer-cgis -- syntax corrected

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

Comment 10

[Marc Schumann [:Wurblzap]]

Attachment: Screenshot of footer

As talked about on irc.

Comment 11

[Marc Schumann [:Wurblzap]]

Attachment: Full error message

Comment 12

[Christian Reis]

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

Comment 13

[Christian Reis]

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

[Christian Reis]

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.

Comment 15

[Christian Reis]

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).

Comment 16

[Dave Miller [:justdave]]

ask for approval again after you get it reviewed :)

Comment 17

[Marc Schumann [:Wurblzap]]

(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.

Comment 18

[Dave Miller [:justdave]]

Sorry, you are correct.  There are non-patch attachments after the patch, which
confused me.

Comment 19

[Joel Peshkin]

Attachment: Same patch as 154108 ported to 2_19

Comment 20

[Joel Peshkin]

checked in on both branches