Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2864043:
Use TimedWait everywhere to prevent hangs. (Closed)
|
Created:
10 years, 5 months ago by skrul Modified:
9 years, 7 months ago CC:
chromium-reviews, ncarter (slow), ben+cc_chromium.org, Raghu Simha, idana, tim (not reviewing), Paweł Hajdan Jr. Visibility:
Public. |
DescriptionUse TimedWait everywhere to prevent hangs.
I'm having a hard time reproing this hang locally -- I ran the test for several days on windows with no hangs. So this change should prevent any individual test in this test from hanging and hopefully help provide some helpful info when it hangs again.
And since the tests won't hang anymore, I believe we can make them FLAKY.
BUG=39070
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51980
Patch Set 1 #Patch Set 2 : Fix typo. #Patch Set 3 : Added TODOs explaining usage of FLAKY. #Messages
Total messages: 11 (0 generated)
Drive-by. Please *don't* do things like that. This just papers over the real issues and makes bad patterns proliferate through the codebase.
I don't plan on leaving things this way, rather this is to help me diagnose the real problem. I've been unable to reproduce the hang locally, and the logs from the buildbots without this fix are not helpful in identifying the problem. The bug will remain on my plate until this is properly fixed. On 2010/07/07 20:08:29, Paweł Hajdan Jr. wrote: > Drive-by. > > Please *don't* do things like that. This just papers over the real issues and > makes bad patterns proliferate through the codebase.
Could you try to borrow a trybot and attempt to reproduce the issue there? If that still fails, please add some TODOs in the code (it's a hard issue, it's possible we'll forget about the issue or get distracted before it can be fixed properly, etc; and there is risk of other people proliferating the pattern thinking it's fine). Also, if you decide to land the change, please let me take another look at the TODOs before you commit.
I sent this change to the windows build bot a few times, we'll see if it fails on it :) I've also added a TODO explaining why I am changing these tests to FLAKY. On 2010/07/07 20:19:40, Paweł Hajdan Jr. wrote: > Could you try to borrow a trybot and attempt to reproduce the issue there? > > If that still fails, please add some TODOs in the code (it's a hard issue, it's > possible we'll forget about the issue or get distracted before it can be fixed > properly, etc; and there is risk of other people proliferating the pattern > thinking it's fine). Also, if you decide to land the change, please let me take > another look at the TODOs before you commit.
So I've run this 7 times on the win trybots and so far it has not flaked. I would really like to check this in and monitor it over a few days and see what happens. Paweł, does this seem OK to you? On 2010/07/07 22:23:56, skrul wrote: > I sent this change to the windows build bot a few times, we'll see if it fails > on it :) I've also added a TODO explaining why I am changing these tests to > FLAKY. > > > On 2010/07/07 20:19:40, Paweł Hajdan Jr. wrote: > > Could you try to borrow a trybot and attempt to reproduce the issue there? > > > > If that still fails, please add some TODOs in the code (it's a hard issue, > it's > > possible we'll forget about the issue or get distracted before it can be fixed > > properly, etc; and there is risk of other people proliferating the pattern > > thinking it's fine). Also, if you decide to land the change, please let me > take > > another look at the TODOs before you commit.
Okay, let's do it that way: please *borrow* a trybot (read internal wiki or ask maruel if you're not sure) and try to reproduce manually. If it still fails to reproduce, feel free to go ahead and land (LGTM). On Thu, Jul 8, 2010 at 17:55, <skrul@chromium.org> wrote: > So I've run this 7 times on the win trybots and so far it has not flaked. > I > would really like to check this in and monitor it over a few days and see > what > happens. > > Paweł, does this seem OK to you? > > > On 2010/07/07 22:23:56, skrul wrote: > >> I sent this change to the windows build bot a few times, we'll see if it >> fails >> on it :) I've also added a TODO explaining why I am changing these tests >> to >> FLAKY. >> > > > On 2010/07/07 20:19:40, Paweł Hajdan Jr. wrote: >> > Could you try to borrow a trybot and attempt to reproduce the issue >> there? >> > >> > If that still fails, please add some TODOs in the code (it's a hard >> issue, >> it's >> > possible we'll forget about the issue or get distracted before it can be >> > fixed > >> > properly, etc; and there is risk of other people proliferating the >> pattern >> > thinking it's fine). Also, if you decide to land the change, please let >> me >> take >> > another look at the TODOs before you commit. >> > > > > http://codereview.chromium.org/2864043/show >
The advantage of having trybot for a while is that you can compile it once, and then just run the test (with a filter) and let it repeat a lot of times (1000 or so). That way you have much higher chance of catching the flake. Good luck. :) On Thu, Jul 8, 2010 at 17:56, Paweł Hajdan, Jr. <phajdan.jr@chromium.org>wrote: > Okay, let's do it that way: please *borrow* a trybot (read internal wiki or > ask maruel if you're not sure) and try to reproduce manually. > > If it still fails to reproduce, feel free to go ahead and land (LGTM). > > > On Thu, Jul 8, 2010 at 17:55, <skrul@chromium.org> wrote: > >> So I've run this 7 times on the win trybots and so far it has not flaked. >> I >> would really like to check this in and monitor it over a few days and see >> what >> happens. >> >> Paweł, does this seem OK to you? >> >> >> On 2010/07/07 22:23:56, skrul wrote: >> >>> I sent this change to the windows build bot a few times, we'll see if it >>> fails >>> on it :) I've also added a TODO explaining why I am changing these tests >>> to >>> FLAKY. >>> >> >> >> On 2010/07/07 20:19:40, Paweł Hajdan Jr. wrote: >>> > Could you try to borrow a trybot and attempt to reproduce the issue >>> there? >>> > >>> > If that still fails, please add some TODOs in the code (it's a hard >>> issue, >>> it's >>> > possible we'll forget about the issue or get distracted before it can >>> be >>> >> fixed >> >>> > properly, etc; and there is risk of other people proliferating the >>> pattern >>> > thinking it's fine). Also, if you decide to land the change, please let >>> me >>> take >>> > another look at the TODOs before you commit. >>> >> >> >> >> http://codereview.chromium.org/2864043/show >> > >
Steve has been trying to repro this manually for many days now, on private machines running for 6 days and on trybots. So far the only place we've actually seen it fail is the buildbot. He's dedicated to fixing it, so maybe the fastest path to fixing it really is to retry it in the real environment? We could borrow a trybot in parallel (using --revision <before this patch lands on trunk>). That said, I'm not the flakiness expert by any means :) On Thu, Jul 8, 2010 at 8:56 AM, Paweł Hajdan, Jr. <phajdan.jr@chromium.org>wrote: > Okay, let's do it that way: please *borrow* a trybot (read internal wiki or > ask maruel if you're not sure) and try to reproduce manually. > > If it still fails to reproduce, feel free to go ahead and land (LGTM). > > > On Thu, Jul 8, 2010 at 17:55, <skrul@chromium.org> wrote: > >> So I've run this 7 times on the win trybots and so far it has not flaked. >> I >> would really like to check this in and monitor it over a few days and see >> what >> happens. >> >> Paweł, does this seem OK to you? >> >> >> On 2010/07/07 22:23:56, skrul wrote: >> >>> I sent this change to the windows build bot a few times, we'll see if it >>> fails >>> on it :) I've also added a TODO explaining why I am changing these tests >>> to >>> FLAKY. >>> >> >> >> On 2010/07/07 20:19:40, Paweł Hajdan Jr. wrote: >>> > Could you try to borrow a trybot and attempt to reproduce the issue >>> there? >>> > >>> > If that still fails, please add some TODOs in the code (it's a hard >>> issue, >>> it's >>> > possible we'll forget about the issue or get distracted before it can >>> be >>> >> fixed >> >>> > properly, etc; and there is risk of other people proliferating the >>> pattern >>> > thinking it's fine). Also, if you decide to land the change, please let >>> me >>> take >>> > another look at the TODOs before you commit. >>> >> >> >> >> http://codereview.chromium.org/2864043/show >> > >
Oh, looks like I may be more disturbing here than really helping. Yeah, feel free to commit. On Thu, Jul 8, 2010 at 18:06, Tim Steele <tim@chromium.org> wrote: > Steve has been trying to repro this manually for many days now, on private > machines running for 6 days and on trybots. So far the only place we've > actually seen it fail is the buildbot. He's dedicated to fixing it, so maybe > the fastest path to fixing it really is to retry it in the real environment? > We could borrow a trybot in parallel (using --revision <before this patch > lands on trunk>). That said, I'm not the flakiness expert by any means :) > > > On Thu, Jul 8, 2010 at 8:56 AM, Paweł Hajdan, Jr. <phajdan.jr@chromium.org > > wrote: > >> Okay, let's do it that way: please *borrow* a trybot (read internal wiki >> or ask maruel if you're not sure) and try to reproduce manually. >> >> If it still fails to reproduce, feel free to go ahead and land (LGTM). >> >> >> On Thu, Jul 8, 2010 at 17:55, <skrul@chromium.org> wrote: >> >>> So I've run this 7 times on the win trybots and so far it has not flaked. >>> I >>> would really like to check this in and monitor it over a few days and see >>> what >>> happens. >>> >>> Paweł, does this seem OK to you? >>> >>> >>> On 2010/07/07 22:23:56, skrul wrote: >>> >>>> I sent this change to the windows build bot a few times, we'll see if it >>>> fails >>>> on it :) I've also added a TODO explaining why I am changing these >>>> tests to >>>> FLAKY. >>>> >>> >>> >>> On 2010/07/07 20:19:40, Paweł Hajdan Jr. wrote: >>>> > Could you try to borrow a trybot and attempt to reproduce the issue >>>> there? >>>> > >>>> > If that still fails, please add some TODOs in the code (it's a hard >>>> issue, >>>> it's >>>> > possible we'll forget about the issue or get distracted before it can >>>> be >>>> >>> fixed >>> >>>> > properly, etc; and there is risk of other people proliferating the >>>> pattern >>>> > thinking it's fine). Also, if you decide to land the change, please >>>> let me >>>> take >>>> > another look at the TODOs before you commit. >>>> >>> >>> >>> >>> http://codereview.chromium.org/2864043/show >>> >> >> >
LGTM |
