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?
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.
Created attachment 239363 [details] Adds a call to task.validate() and handles invalid tasks in the same way that EWS does.
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?
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.
*** Bug 137481 has been marked as a duplicate of this bug. ***
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.
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. :) )
I found one more nasty bug related to this one: bug137483
> 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.
(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.
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.
(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.
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.
(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.
Created attachment 239428 [details] Integrates Csaba's unit test, and makes the validation codepath use exceptions to report invalid patches.
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.
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.
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>
All reviewed patches have been landed. Closing bug.