All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.
Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.
Review Request #395 - Created July 13, 2011 and updated
| Jenn | Reviewers | ||
| stone.linden, nat_linden | |||
| None | viewer-development | ||
Update watchdog timer to 60 second timeout for ongoing use. Also, update setting description to be more clear.
Tested with latest updates - on infinite loop, it quits out after 60 seconds as expected. Followed testplan here: https://wiki.lindenlab.com/wiki/Watchdog_Status_Test_Plan
None yet.
Hmm, tested this, and times out at 20 seconds rather than 60. Upon investigation, it appears this may be hardcoded inside Watchdog thread. Looking into it further.
Hold review until hardcoded behavior updated or changed.
I'm okay with this change -- but does it actually change watchdog behavior? Roxie was saying yesterday in IRC that it looks to her as though the code is setting the actual length of the timeout to MainloopTimeoutDefault, which happens to be 20 seconds, which happens to match the previous value of the watchdog timer.
Hmm, tested this, and times out at 20 seconds rather than 60. Upon investigation, it appears this may be hardcoded inside Watchdog thread. Looking into it further.
Tested with latest updates - on infinite loop, it quits out after 60 seconds as expected. Followed testplan here: https://wiki.lindenlab.com/wiki/Watchdog_Status_Test_Plan
Diff updated with changes that *actually* change the watchdog value. Turns out 'MainloopTimeoutDefault' is the setting to control the actual timeout value in seconds. Tested changes, works as expected.
| indra/newview/app_settings/settings.xml (Diff revision 2) |
|---|
I realize we have to deal with legacy settings files, but if we must use two different settings for this functionality it'd be nice if their names bore some resemblance. Do we know whether MainloopTimeoutDefault has any other role? Could we set that to -1 to disable, and get rid of WatchdogEnabled? Or could we make WatchdogEnabled behave the way everyone expects, that is, either -1 or timeout in seconds?
| indra/newview/app_settings/settings.xml (Diff revision 2) |
|---|
If this is boolean-valued, I'd sure rather see it be boolean type rather than "int with special values 1 or -1." Even if it has to stay int, I'd rather see 1 and 0...
Expand All
Other reviews