RESOLVED FIXED 137460
Commit queue doesn't drop obsolete patches sometimes
https://bugs.webkit.org/show_bug.cgi?id=137460
Summary Commit queue doesn't drop obsolete patches sometimes
Alexey Proskuryakov
Reported 2014-10-06 13:50:28 PDT
Bug 137364 is long closed, but commit queue is still orange, and keeps rolling. It made over 40 thousands attempts already, isn't that fun?
Attachments
Adds a call to task.validate() and handles invalid tasks in the same way that EWS does. (2.03 KB, patch)
2014-10-06 15:53 PDT, Jake Nielsen
ossy: review+
ossy: commit-queue-
unittest for the patch (2.74 KB, patch)
2014-10-07 04:18 PDT, Csaba Osztrogonác
no flags
Integrates Csaba's unit test, and makes the validation codepath use exceptions to report invalid patches. (13.58 KB, patch)
2014-10-07 12:45 PDT, Jake Nielsen
no flags
Alexey Proskuryakov
Comment 1 2014-10-06 13:51:40 PDT
I don't know if I broke this with a recent refactoring, or if it's always been like this. Jake has graciously agreed to look into this.
Jake Nielsen
Comment 2 2014-10-06 15:53:33 PDT
Created attachment 239363 [details] Adds a call to task.validate() and handles invalid tasks in the same way that EWS does.
Daniel Bates
Comment 3 2014-10-06 15:55:52 PDT
Comment on attachment 239363 [details] Adds a call to task.validate() and handles invalid tasks in the same way that EWS does. Is it possible to write a unit test for this change?
Alexey Proskuryakov
Comment 4 2014-10-06 15:56:56 PDT
Comment on attachment 239363 [details] Adds a call to task.validate() and handles invalid tasks in the same way that EWS does. I'm curious why validate() calls in CommitQueueTask don't work.
Alexey Proskuryakov
Comment 5 2014-10-07 00:32:17 PDT
*** Bug 137481 has been marked as a duplicate of this bug. ***
Csaba Osztrogonác
Comment 6 2014-10-07 03:56:21 PDT
Comment on attachment 239363 [details] Adds a call to task.validate() and handles invalid tasks in the same way that EWS does. I checked, it is the right fix, EWS and style queue do the same thing. task.run() also calls validate, but it simply returns with false and then the patch is unlocked, but not removed from the queue. We should validate at the beginning and then call _did_error to remove the task from the queue. r=me, but I agree, we should add a unittest for it.
Csaba Osztrogonác
Comment 7 2014-10-07 04:18:03 PDT
Created attachment 239404 [details] unittest for the patch I played a little bit how to unittest this fix, and verified that the proposed fix is really fix the issue. output without the proposed fix: - MOCK: release_lock: commit-queue 10007 output with the proposed fix: + MOCK: update_status: commit-queue Error: commit-queue did not process patch. + MOCK: release_work_item: commit-queue 10007 Feel free to integrate it to the original patch. ( I only ask for a credit in the changelog. :) )
Csaba Osztrogonác
Comment 8 2014-10-07 04:35:26 PDT
I found one more nasty bug related to this one: bug137483
Alexey Proskuryakov
Comment 9 2014-10-07 09:38:15 PDT
> task.run() also calls validate, but it simply returns with false and then the patch is unlocked So why is that the right thing? The fact that EWS and style queue do the same thing doesn't mean that they don't have a bug, too.
Csaba Osztrogonác
Comment 10 2014-10-07 09:53:48 PDT
(In reply to comment #9) > > task.run() also calls validate, but it simply returns with false and then the patch is unlocked > > So why is that the right thing? The fact that EWS and style queue do the same thing doesn't mean that they don't have a bug, too. The task doesn't know anything about how the queues work. They simple do their tasks, validate and run the task and return with False or True. Check commitqueuetask.py, stylequeuetask.py and commitqueuetask.py The upper layer should handle the False return value of task.validate(), as StyleQueue.review_patch() and AbstractEarlyWarningSystem.review_patch() do properly at the beginning. But CommitQueue.process_work_item didn't do it before at all. The proposed patch adds this necessary check.
Alexey Proskuryakov
Comment 11 2014-10-07 10:33:18 PDT
What is the purpose of calling validate() in review_patch()? All tasks' run() methods already do that - often several times - so it seems like we should make sure that the result of the existing check is not ignored, not add another check.
Csaba Osztrogonác
Comment 12 2014-10-07 11:59:22 PDT
(In reply to comment #11) > What is the purpose of calling validate() in review_patch()? All tasks' run() methods already do that - often several times - so it seems like we should make sure that the result of the existing check is not ignored, not add another check. run() returns with False or True. You can't determine why it returned with False. The reason can be: not valid patch, unable to build or update repository, ... But if you call validate() before calling run(), you could simply remove the patch from the queue if it isn't valid. Everybody do this, except the CQ.
Alexey Proskuryakov
Comment 13 2014-10-07 12:17:44 PDT
Yes, certainly. What I'm saying is that we should at the very least delete the first call to validate() inside run() if we are adding it before run(). There is no reason to call validate twice it a row. However, Jake is going to make validate() raise an exception, so that the caller would know what actually happened.
Csaba Osztrogonác
Comment 14 2014-10-07 12:23:58 PDT
(In reply to comment #13) > Yes, certainly. What I'm saying is that we should at the very least delete the first call to validate() inside run() if we are adding it before run(). There is no reason to call validate twice it a row. I agree. > However, Jake is going to make validate() raise an exception, so that the caller would know what actually happened. It is a little bigger refactoring, but let's see. If we would like to make validate() to raise excpeption, we could make it private (_validate) and get rid of the two existing validate call before calling run.
Jake Nielsen
Comment 15 2014-10-07 12:45:21 PDT
Created attachment 239428 [details] Integrates Csaba's unit test, and makes the validation codepath use exceptions to report invalid patches.
Alexey Proskuryakov
Comment 16 2014-10-07 13:29:15 PDT
Comment on attachment 239428 [details] Integrates Csaba's unit test, and makes the validation codepath use exceptions to report invalid patches. View in context: https://bugs.webkit.org/attachment.cgi?id=239428&action=review > Tools/ChangeLog:26 > + Remove call to validate() and instead catches the PatchIsNotValid > + exception. Should we also remove the call to validate() in StyleQueue.review_patch()? This patch changes commit queue and EWS, including some code that is shared between style queue and EWS, but it doesn't fully clean up the style queue.
Alexey Proskuryakov
Comment 17 2014-10-07 13:32:59 PDT
Comment on attachment 239428 [details] Integrates Csaba's unit test, and makes the validation codepath use exceptions to report invalid patches. View in context: https://bugs.webkit.org/attachment.cgi?id=239428&action=review >> Tools/ChangeLog:26 >> + exception. > > Should we also remove the call to validate() in StyleQueue.review_patch()? This patch changes commit queue and EWS, including some code that is shared between style queue and EWS, but it doesn't fully clean up the style queue. Actually, I guess it doesn't touch the style queue.
WebKit Commit Bot
Comment 18 2014-10-07 15:14:02 PDT
Comment on attachment 239428 [details] Integrates Csaba's unit test, and makes the validation codepath use exceptions to report invalid patches. Clearing flags on attachment: 239428 Committed r174408: <http://trac.webkit.org/changeset/174408>
WebKit Commit Bot
Comment 19 2014-10-07 15:14:07 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.