When locking tables, namedqueries READ and whine_queries READ are always required due to Throw{Code|User}Error() (well... only if these subroutines are called). If this does not break anything, I suggest that _throw_error always do a UNLOCK TABLES instead of using the $unlock_tables variable. As this subroutine always ends with an "exit", this sounds reasonable. Note that MySQL does not complains if UNLOCK TABLES is used without a previous LOCK TABLES.

bbaetz, could you please tell us if it is reasonable to unconditionally unlock tables in Throw*Error()?

In MySQL, it is probably ok, but it will cause problems on other databases. E.g. on Postgres, you are unlocking DB by closing transaction. Transaction close, rollback in this case, will trigger DBI error when the transaction is not started first. As it would be quite complex to keep track if any tables are locked at the moment or not for error handling with the current implementation, I would suggest postponing this until bug 237862 is in, then it will be much simpler to do properly, as the locking/unlocking code will be centralised to the compatibility layer.

I agree with Tomas.

(In reply to comment #2) > As it would be quite complex to keep track if any tables are locked at the > moment or not for error handling with the current implementation, I would > suggest postponing this until bug 237862 is in. I'm fine with that.

I am taking this, feel free to take it back if you want :-).

Comment on attachment 174823 [details] [diff] [review] V1 OK, this *looks* good and correct, but somebody needs to actually do a bit of testing on it, if possible. :-)

Sorry for the late change, this one does not change anything in the patch itself, it just adds one more unlock I found to be missing before the code die()s.

Can someone take a look at this, before it bitrotts?

Comment on attachment 174838 [details] [diff] [review] V1.1 Really sorry, but I've got to r- this -- not because of what you're doing, but because of what you've done along the way to what you're doing. I completely agree that all of the extraneous 'exit;' statements need to be removed from the code, but IMHO code cleanup should be its own bug, and not lumped together with something else. Removing them here (is this all of them?) really obfuscates this patch, and is not relevant to what you're really trying to accomplish. Please splt the patch into two; one half that is directly related to what you're trying to accomplish -- namely, unlocking the tables every time Throw*Error is called -- then open up a new bug for the extraneous exits and land the other half of the patch there. I don't think I saw anything wrong with what you were trying to do with this patch, but I need to see it more tightly focused to be 100% sure. Also, just FMI, are you certain that you have caught ALL instances of the third parameter for Call*Error? Are they all called "abort"?

Oh, and just FYI, there are already two sections that FAILED to apply; patching file editcomponents.cgi Hunk #6 FAILED at 286. patching file process_bug.cgi Hunk #6 FAILED at 1275.

Well, I tought it was pretty simple, every Throw*Error call does exit internally, so no exit needs to be called afterwards, but I split the patch as requested. It's also updated now. > I don't think I saw anything wrong with what you were trying to do with this > patch, but I need to see it more tightly focused to be 100% sure. Also, just > FMI, are you certain that you have caught ALL instances of the third parameter > for Call*Error? Are they all called "abort"? Dunno, I was not greping for 'abort' :-). But I believe I found all, I was careful...

Comment on attachment 176032 [details] [diff] [review] V1.2 With regards to the 'abort' question... I'm glad to hear you didn't just do a global grep. I was asking (albeit rather poorly, and in a roundabout way) about your methodology, and I'm glad to hear it was thorough. - ThrowUserError('product_not_specified'); + ThrowUserError('product_not_specified'); Note: Whitespace change only. Please remove from patch before applying. Other than that, I can see no problems from this patch. The issue, of course, is that it's extremely difficult to do any significant code testing on other than a soak-test. Obviously there will be no problems in any of the Throw*Errors from which you removed the third parameter, or else they'd have already happened; the place that problems will likely arise is from Throw*Error calls that did NOT have a third parameter, but are now unlocking the DB anyway because it's done as a matter of course. I cannot think of any troubles that could cause, but then again I am not a DB specialist. If they arise, we'll mark them and link them to this bug, and you can fix them. :) r+ by inspection and analysis.

Note: It's OK for me to have a single patch which also removes useless 'exit'.

Comment on attachment 176032 [details] [diff] [review] V1.2 Nobody has tested this patch. I don't feel confident with a simple 'r+ by inspection'.

(In reply to comment #15) > (From update of attachment 176032 [details] [diff] [review] [edit]) > Nobody has tested this patch. I don't feel confident with a simple 'r+ by > inspection'. > I am running older version of this patch for a while now without any problems...

(In reply to comment #13) > With regards to the 'abort' question... I'm glad to hear you didn't just do a > global grep. I was asking (albeit rather poorly, and in a roundabout way) about > your methodology, and I'm glad to hear it was thorough. greping for ThrowCodeError, ThrowUserError and ThrowTemplateError and inspecting each manually. Well, at the end I did grep for abort, just to confirm I haven't overlooked anything :-). > Obviously there will be no problems in any of the > Throw*Errors from which you removed the third parameter, or else they'd have > already happened; the place that problems will likely arise is from Throw*Error > calls that did NOT have a third parameter, but are now unlocking the DB anyway > because it's done as a matter of course. I cannot think of any troubles that > could cause, but then again I am not a DB specialist. If they arise, we'll mark > them and link them to this bug, and you can fix them. :) I am pretty sure this will not cause any trouble. When you call Throw*Error, the call does not return, you are finished with processing, so there is no problem with subsequent code expecting the tables being still locked. If there were no locks, nothing happens. If there were locks, they are unlocked before the exit. I am think this is the correct behaviour ;-).

Comment on attachment 176032 [details] [diff] [review] V1.2 Works fine. No problem detected and all Throw*Error have been correctly considered. r=LpSolit

Checking in CGI.pl; /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v <-- CGI.pl new revision: 1.231; previous revision: 1.230 done Checking in editclassifications.cgi; /cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v <-- editclassifications.cgi new revision: 1.6; previous revision: 1.5 done Checking in editcomponents.cgi; /cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v <-- editcomponents.cgi new revision: 1.49; previous revision: 1.48 done Checking in editmilestones.cgi; /cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v <-- editmilestones.cgi new revision: 1.32; previous revision: 1.31 done Checking in editusers.cgi; /cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v <-- editusers.cgi new revision: 1.80; previous revision: 1.79 done Checking in editversions.cgi; /cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v <-- editversions.cgi new revision: 1.31; previous revision: 1.30 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.312; previous revision: 1.311 done Checking in post_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v <-- post_bug.cgi new revision: 1.104; previous revision: 1.103 done Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.240; previous revision: 1.239 done Checking in Bugzilla/Bug.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v <-- Bug.pm new revision: 1.61; previous revision: 1.60 done Checking in Bugzilla/Error.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v <-- Error.pm new revision: 1.12; previous revision: 1.11 done Checking in Bugzilla/Auth/Login/WWW/CGI.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/CGI.pm,v <-- CGI.pm new revision: 1.8; previous revision: 1.7 done