Bug 137460 – Commit queue doesn't drop obsolete patches sometimes

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

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]

Created attachment 239363 [details]
Adds a call to task.validate() and handles invalid tasks in the same way that EWS does.

[Daniel Bates]

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

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

[Csaba Osztrogonác]

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]

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]

I found one more nasty bug related to this one: bug137483

[Alexey Proskuryakov]

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

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

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]

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

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]

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

Created attachment 239428 [details]
Integrates Csaba's unit test, and makes the validation codepath use exceptions to report invalid patches.

[Alexey Proskuryakov]

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

All reviewed patches have been landed.  Closing bug.