Bug 155760

Summary: imported/w3c/web-platform-tests/streams/readable-streams/general.https.html is a flaky failure
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Tools / TestsAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jiewen_tan, lforschler, ryanhaddad, youennf
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Alexey Proskuryakov 2016-03-22 11:24:40 PDT
imported/w3c/web-platform-tests/streams/readable-streams/general.https.html fails somewhat frequently:

@@ -13,7 +13,7 @@
 PASS ReadableStream should be able to enqueue different objects. 
 PASS ReadableStream: if pull rejects, it should error the stream 
 PASS ReadableStream: should only call pull once upon starting the stream 
-PASS ReadableStream: should call pull when trying to read from a started, empty stream 
+FAIL ReadableStream: should call pull when trying to read from a started, empty stream assert_equals: pull should be called when read is called expected 2 but got 1
 PASS ReadableStream: should only call pull once on a non-empty stream read from before start fulfills 
 PASS ReadableStream: should only call pull once on a non-empty stream read from after start fulfills 
 PASS ReadableStream: should call pull in reaction to read()ing the last chunk, if not draining 

http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fstreams%2Freadable-streams%2Fgeneral.https.html
Comment 1 Alexey Proskuryakov 2016-03-24 18:16:38 PDT
It's also frequently reported as a crash on EWS, although I'm not sure if it's this test's fault.
Comment 2 Jiewen Tan 2016-04-13 11:52:40 PDT
Committed r199507: <http://trac.webkit.org/changeset/199507>
Comment 3 Jiewen Tan 2016-04-13 11:53:11 PDT
Reopen to track the fix.
Comment 4 youenn fablet 2016-04-18 10:46:25 PDT
It seems that on slow bots, the order of the promise resolution does not play well with DOM timers.
It seems that this can be solved by changing delay(1) to delay(10) in imported/w3c/web-platform-tests/streams/readable-streams/general.js.
Comment 5 Alexey Proskuryakov 2016-04-18 16:00:08 PDT
Will that make any tests take 10 seconds as part of normal execution, or is that code just to handle failing tests nicely?

Having tests take 10 seconds would be pretty horrible.
Comment 6 youenn fablet 2016-04-19 00:58:00 PDT
Created attachment 276705 [details]
Patch
Comment 7 youenn fablet 2016-04-19 01:02:33 PDT
(In reply to comment #5)
> Will that make any tests take 10 seconds as part of normal execution, or is
> that code just to handle failing tests nicely?
> 
> Having tests take 10 seconds would be pretty horrible.

It is 10 milliseconds.
The issue can probably be summarized to the following code:

  let pullCount1 = 0;
  let pullCount2 = 0;
  Promise.resolve().then(() => {
    ++pullCount1;
    Promise.resolve().then(() => {
      ++pullCount2;
    });
  });

  return delay(1).then(() => {
      // Not flaky
      assert_equals(pullCount1, 1);
      // May be flaky
      assert_equals(pullCount2, 1);
  });

By going to delay(10), the flakiness does not happen anymore on my machine.
The test could probably be improved to further limit the flakiness (returning a resolved promise to the start method).
This can be tackled directly when upstreaming that change to W3C WPT repo.
Comment 8 Alexey Proskuryakov 2016-04-19 10:34:05 PDT
Comment on attachment 276705 [details]
Patch

Seems fine as a temporary workaround.

Generally speaking, bots tend to still have flakiness when increasing timer delays from 1ms to 100ms, because that's just how much variability we get when running the tests. We can't really have any non-zero timeouts as part of normal test flow, they are only useful when catching failures (and then set them to 20 seconds or so).
Comment 9 youenn fablet 2016-04-19 11:17:33 PDT
(In reply to comment #8)
> Comment on attachment 276705 [details]
> Patch
> 
> Seems fine as a temporary workaround.
> 
> Generally speaking, bots tend to still have flakiness when increasing timer
> delays from 1ms to 100ms, because that's just how much variability we get
> when running the tests. We can't really have any non-zero timeouts as part
> of normal test flow, they are only useful when catching failures (and then
> set them to 20 seconds or so).

Thanks for the review.

10 ms is used in several streams tests without causing issue apparently.
I'll land it and see whether this is a good enough fix.
If so, I will upstream this change or a more deterministic version of the test to WPT or streams repo
Comment 10 WebKit Commit Bot 2016-04-19 12:06:30 PDT
Comment on attachment 276705 [details]
Patch

Clearing flags on attachment: 276705

Committed r199736: <http://trac.webkit.org/changeset/199736>
Comment 11 WebKit Commit Bot 2016-04-19 12:06:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 youenn fablet 2016-06-21 00:06:44 PDT
test is still flaky, although less than before.
Let's fix it properly.
Comment 13 youenn fablet 2016-06-21 00:08:03 PDT
Created attachment 281722 [details]
Patch
Comment 14 youenn fablet 2016-06-21 08:04:40 PDT
*** Bug 153095 has been marked as a duplicate of this bug. ***
Comment 15 WebKit Commit Bot 2016-06-22 11:14:21 PDT
Comment on attachment 281722 [details]
Patch

Clearing flags on attachment: 281722

Committed r202338: <http://trac.webkit.org/changeset/202338>
Comment 16 WebKit Commit Bot 2016-06-22 11:14:24 PDT
All reviewed patches have been landed.  Closing bug.