Closed Bug 540617 Opened 10 years ago Closed 10 years ago

Process.kill instantiates another Automation(), which adds another log handler

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9.3a4

People

(Reporter: jruderman, Assigned: jmaher)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Process.kill instantiates another Automation() in order to read IS_WIN32:
http://hg.mozilla.org/mozilla-central/annotate/ffab97de1041/build/automation.py.in#l171

Which has the side effect of adding another log handler, duplicating all output:
http://hg.mozilla.org/mozilla-central/annotate/ffab97de1041/build/automation.py.in#l135

This is a regression from the patch in bug 530475.

Process.kill could check the OS directly, or take the existing Automation object as an argument.  The latter would also make it possible to fix the "self.log.info" line in Process.kill, which looks to me like it gets the wrong "self" and won't work.

Or Automation() could be smarter about when it puts its logging handler in place, so instantiating an extra one doesn't have such weird side effects.
ok,I am testing a patch to fix this.

In python, a inner class does not have access to any members or methods of the outer class.  So to fix this I move log back out of the automation class and reference the global variable during __init__ for both automation and process.
Assignee: nobody → jmaher
Presumably this is what has been causing the triple-logging?
the triple logging has been seen for the automationutils.processLeakLog output.  I see double-logging in the logs and on my linux desktop.  With my fix I am testing it only outputs a single log of output.  I will verify with the results on the tryserver.
fixed mochitest output to only instantiate one instance of the logger in automation.py.  This passes on try server and by verifying the logs on tryserver I do not see the triple-logging style output.
Attachment #422401 - Flags: review?(ted.mielczarek)
Comment on attachment 422401 [details] [diff] [review]
only instantiate one instance of log in automation.py

>diff -r 0f2db07ea587 build/automation.py.in
>--- a/build/automation.py.in	Tue Jan 19 11:42:40 2010 -0500
>+++ b/build/automation.py.in	Tue Jan 19 12:51:26 2010 -0500
>@@ -71,6 +71,15 @@
> #expand _IS_DEBUG_BUILD = __IS_DEBUG_BUILD__
> #expand _CRASHREPORTER = __CRASHREPORTER__ == 1
> 
>+# We use the logging system here primarily because it'll handle multiple
>+# threads, which is needed to process the output of the server and application
>+# processes simultaneously.
>+_log = logging.getLogger()
>+handler = logging.StreamHandler(sys.stdout)
>+_log.setLevel(logging.INFO)
>+_log.addHandler(handler)

Any reason to call this "_log" and not just "log"? (At least make "handler" named consistently with whatever you pick.)

>@@ -130,16 +139,8 @@
>   # timeout, in seconds
>   DEFAULT_TIMEOUT = 60.0
> 
>-  log = logging.getLogger()

Does setting log = _log here not work? Do you have to set it in __init__ ? (Or do you just want to do that so you can override it in your subclass?)

r=me either way
Attachment #422401 - Flags: review?(ted.mielczarek) → review+
I already reported this bug in bug 573739, before m-c check-in :-/

Now I see doubled leak stats or the like, i.e.
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1264894221.1264895863.4401.gz
Linux comm-central-trunk debug test mochitest-other on 2010/01/30 15:30:21

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | missing output line for total leaks!
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | missing output line for total leaks!
}
Status: NEW → ASSIGNED
Depends on: 537739
OS: Mac OS X → All
Hardware: x86 → All
> I already reported this bug in bug 573739,

Typo for bug 537739.
updated for bitrot since this is landing after my other refactoring changes.
Attachment #422401 - Attachment is obsolete: true
Attachment #432348 - Flags: review+
updated for bitrot, now that my other patches.  This patch passes on try server.
Attachment #432348 - Attachment is obsolete: true
Attachment #432813 - Flags: review+
Landed as changeset 0c3f8048f40b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1268951466.1268953048.10535.gz&fulltext=1
Linux comm-central-trunk debug test mochitest-other on 2010/03/18 15:31:06

V.Fixed
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
Target Milestone: --- → mozilla1.9.3a4
You need to log in before you can comment on or make changes to this bug.