293985 - [BUGZILLA] FetchBug repeats bug info in last channel if a product has more than one channel

Status: VERIFIED FIXED

(Webtools Graveyard :: Mozbot, defect, P1)

Description

[Wolf [:wolf]]

While trying to make firebot use the new bzbot-into-mozbot code from bug 292492.
I ran into a nice bug. If you specify more than 1 channel per product (comma
seperated. such as 'Firefox => #firefox,#firebot,#bugs' When a bug change comes
into the module, the FetchBug data is dumped into the last channel 3 times. (I
tried changing the order, to #firefox,#bugs,#firebot, and it was #firebot which
got it instead of #bugs, but the bug persisted.)

It should have output the FetchBug data to #firefox, #firebot, and #bugs though.
:-) Works fine with only 1 channel though.

I'm guessing the bug is somewhere in this code, bug I'm not sure what. Its like
the target isn't getting an updated $channel, and just the last one.

# We can't use "local" here, or the target doesn't show
# up properly in the GotURI after FetchBug.
$event->{'target'} = $channel;
$self->say($event, $message);
$self->FetchBug($event, $bug_id, 'bug')
    unless $said_bug{$channel . $bug_id};
$said_to{$channel} = 1;

Comment 1

[Max Kanat-Alexander]

That's very strange. Because of the $said_bug check, I'm fairly certain that
$channel is different every time. (Otherwise the unless would trigger, and the
FetchBug wouldn't happen.)

This is possibly a bug in GotURI, whichever bug is causing the bot to also
sometimes address the FetchBug notification to the admin instead of it having no
prefix.

Does he say the actual update notification in all three channels?

Comment 2

[Wolf [:wolf]]

(In reply to comment #1)
> Does he say the actual update notification in all three channels?

Yeah, he does. Which is what surprised me. He says the Update notification in
all 3 just fine, then the bug info comes, all into the last channel. heh.

Comment 3

[Colin Ogilvie [:cso]]

Attachment: Log File

This attachment shows the log from Mozbot - I added extra statements to show
what its doing when I ran the script and then removed the time to make it
smaller.

It looks to me as if the $event that GotURI gets has been modified since the
call to FetchBug...

Comment 4

[Colin Ogilvie [:cso]]

It seems when spawnChild is called, $event->{'target'} is correct - each time.

However, when ChildCompleted is called, $event->{'target'} is wrong, both
times...  its too late for me to do more debugging just now though.

Hixie, do you have any ideas why this might be?

Comment 5

[Hixie (not reading bugmail)]

Not off-hand, but it's been ages since I had a change to look at the mozbot
code, sorry.

Comment 6

[Max Kanat-Alexander]

I suspect that $event is being recycled somehow within the mozbot code, so that it's not thread-safe. The trick is just to find out where it is that we're failing to re-create $event.

Comment 7

[Teemu Mannermaa (:wicked)]

Attachment: Simple fix to preserve event in spawnChild, V1

This seems to do the trick. Problem was that spawnChild() sub in mozbot.pl saved same event hash to the pipe as it continued to use for rest of processing. Saved, but now modified event hash, was again used when the child finished.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 207817 [details] [diff] [review]
Simple fix to preserve event in spawnChild, V1

Looks okay to me. At the least, [%$event} definitely couldn't hurt anything. But -- why'd you modify doScheduled?

Comment 9

[Max Kanat-Alexander]

Oh and also -- Hixie: I know that I technically probably can't r+ mozbot core code, but that really is a trivial and obvious change, and I didn't particularly expect that you'd have time to look it over, yes? (It's just one line, though.)

Comment 10

[Colin Ogilvie [:cso]]

Applied this to ssdbot; lets see what happens...

Comment 11

[Teemu Mannermaa (:wicked)]

(In reply to comment #8)
> But -- why'd you modify doScheduled?

Just made the commented out debug code work. I did find it usefull when I was trying to figure out logical flow of mozbot code. :)

I was thinking of adding more debug output that would more clearly show why we need this one line change. But then I didn't want to add even more output to mozbot. But simply put, we need to make a separate copy of the event hash to preserve it's content. So we dereference the hashref to get a hash that we use to initialize a anonymous hashref that get saved to the pipe information used later. Maybe this line needs a better comment when/if checked in?

Comment 12

[Hixie (not reading bugmail)]

This is a pretty big change. Why would the event hash change? It's been a while since I looked at this code, sadly, but isn't a new hash created for each event?

Comment 13

[Teemu Mannermaa (:wicked)]

(In reply to comment #12)
> This is a pretty big change. Why would the event hash change? It's been a while
> since I looked at this code, sadly, but isn't a new hash created for each
> event?

There is at least one case in Bugzilla.bm module that the event hash is changed and used later in different time. The CheckForBugMail sub has a loop for multiple channels which calls FetchBug sub multiple times. This in turn calls getURI which calls spawnChild (both in in mozbot.pl) and thus can spawn is called multiple times from a single event. The target channel is stored in the event hash so when these children eventually come back to GotURI they will have same target channel and we have this problem in our hands.

So, basically, every time getURI/spawnChild is called multiple times during one event we need to make sure each gotURI/ChildCompleted will get an unique copy of event hash to work with.

Does this description help you at all? This change has been live in our bots and seems to be working and is, at least for this case, very much needed.

Comment 14

[Teemu Mannermaa (:wicked)]

Let's face it, MozBot is dead and so there's no chance I can ever get these changes done.

Comment 15

[Wolf [:wolf]]

Resetting to new default owner.

Comment 16

[Wolf [:wolf]]

Checking in mozbot.pl;
/cvsroot/mozilla/webtools/mozbot/mozbot.pl,v  <--  mozbot.pl
new revision: 2.55; previous revision: 2.54
done

Comment 17

[Tanner M. Young [:tmyoung]]

Verified Fixed.