Closed Bug 255772 Opened 19 years ago Closed 19 years ago

whine.pl can get into an endless whining loop

Categories

(Bugzilla :: Whining, defect)

2.19
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: jussi, Assigned: erik)

Details

Attachments

(1 file, 2 obsolete files)

whine.pl can set the run_next field to today which causes an endless mailing
(whining) loop. For me it ment about 5000 emails before mysqld was restarted and
whining removed. I'm living in an area where the first day of the week is monday
but I'm not sure how it could be a problem here. Here's some prints from
reset_timer and get_next_date subroutines:

$run_day: Mon
$run_today: 1
$run_time: 1
$now_hour: 17
$now_weekday: 1
$day_num: 1
$add_days: 0
$nextdate: 2004-08-16

The $nextdate should propably have a date which is a week from now.

$ date
Mon Aug 16 16:56:56 EEST 2004

And here's the relevant parts of the whining related tables. The schedules 4 and
6 went into the mailing loop. Schedule 1 has a query which does not return any
bugs and schedule 3 seemed to be ok.

mysql> select * from whine_schedules;
+----+---------+---------+----------+---------------------+---------------+
| id | eventid | run_day | run_time | run_next            | mailto_userid |
+----+---------+---------+----------+---------------------+---------------+
|  1 |       1 | Sun     | 12       | 2004-08-22 12:00:00 |             1 |
|  3 |       3 | MF      | 1        | 2004-08-17 01:00:00 |             1 |
|  4 |       3 | Mon     | 1        | 2004-08-16 01:00:00 |             2 |
|  6 |       3 | Mon     | 1        | 2004-08-16 01:00:00 |             5 |
+----+---------+---------+----------+---------------------+---------------+
4 rows in set (0.00 sec)

mysql> select id,eventid,onemailperbug from whine_queries;
+----+---------+---------------+
| id | eventid | onemailperbug |
+----+---------+---------------+
|  1 |       1 |             0 |
|  4 |       3 |             0 |
+----+---------+---------------+
2 rows in set (0.00 sec)

mysql> select id,owner_userid from whine_events;
+----+--------------+
| id | owner_userid |
+----+--------------+
|  1 |            1 |
|  3 |            1 |
+----+--------------+
2 rows in set (0.00 sec)

The fix could be to change this comparison in get_next_date to use <=

        if ($add_days < 0) { # it's next week
            $add_days += 7;
        }
Eww!
Severity: major → critical
Target Milestone: --- → Bugzilla 2.20
Attached patch Comparison fix (obsolete) — Splinter Review
This is the fix proposed in comment 0.
Assignee: erik → jussi
Status: NEW → ASSIGNED
(In reply to comment #2)
> Created an attachment (id=156273)
> Comparison fix
> 
> This is the fix proposed in comment 0.

It looks good.  I have a patch that I'm testing right now that should safeguard
against future unforseen infinite loops, and I'd like to work the two together
if possible.
Attachment #156273 - Flags: review?(erik)
(In reply to comment #3)
> It looks good.  I have a patch that I'm testing right now that should safeguard
> against future unforseen infinite loops, and I'd like to work the two together
> if possible.

Go ahead. I just had the patch available and a safeguard in a cron executed
script sounds like a good idea.
Assignee: jussi → erik
Status: ASSIGNED → NEW
Attached patch comparison fix plus safeguard (obsolete) — Splinter Review
This is Jussi's patch with an added safeguard that stops any schedule from
being run more than once by a single execution of the program, which is
something I'd like to get rolled into place so that any more of my brainos in
whine.pl won't cause people to get 50,000 messages.
Attachment #156273 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #156274 - Flags: review?
Comment on attachment 156274 [details] [diff] [review]
comparison fix plus safeguard

>Index: whine.pl
>===================================================================
>+# touched by reset_timer.  If reset_timer sees one schedule more than once, it

Nit: s/one/a/

>+# sets it to NULL so it won't come up again.

"sets run_next to NULL so it won't come up again until the next execution of
whine.pl"?

>+    if (grep(/^$schedule_id$/, @seen_schedules)) {
>+        null_schedule($schedule_id);
>+    }

Where exactly do you push schedule ids into @seen_schedules? (yeah...)
Attachment #156274 - Flags: review? → review-
Attached patch fixed patchSplinter Review
Great.	One embarrassing oversight replaced by another.  Here's one that should
actually do something.
Attachment #156274 - Attachment is obsolete: true
Attachment #156280 - Flags: review?(jouni)
Attachment #156273 - Flags: review?(erik)
Comment on attachment 156280 [details] [diff] [review]
fixed patch

>+# @seen_schedules is a list of all of the schedules that have already been
>+# touched by reset_timer.  If reset_timer sees a schedule more than once, it
>+# sets it to NULL so it won't come up again until the next execution of
>+# whine.pl

Nit: Add a full stop at the end.


My CVS access is still dead since pserver got closed, so I can't actually try
this against the tip. If it passes tests and works for you, r=jouni with the
change above.
Attachment #156280 - Flags: review?(jouni) → review+
Attachment #156280 - Flags: review?
Attachment #156280 - Flags: review?
Flags: approval?
Flags: approval? → approval+
checked in for erik
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
There's still another < to change. It covers the case when run_day has been set
to a day in month:

<http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/whine.pl#652>

Also, in this case the seen check doesn't seem to work and I'm guessing it needs
a return or otherwise the date will be reset before reset_timer returns.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.