Re: [eventsource-tests] [eventsource] Do not send invalid HTTP response (#5037)

After still further research, I have learned that this behavior is neither a
bug in the tests nor in Chromium but actually a bug in the testing
infrastructure itself.

It seems that Python request handlers share some state. This allows the
execution of one handler to effect the response of independent requests. I've
published a reduced demonstration on the following branch:

https://github.com/w3c/web-platform-tests/compare/master...bocoup:serve-bug-demo

The reduction may make this condition seem kind of arcane, but this basic
behavior is what caused the race condition that this patch is intended to
avoid. In the effected test, the browser creates an `EventSource` object, and
the server responds with an HTTP status of 204 ("No content"). It happens to
include a body (in violation of the specification), but Chrome is generally
tolerant of this.

The problem is observable if the body happens to be written to the wire in a
separate TCP frame. (This is where the flakiness comes in since this detail is
opaque to the HTTP protocol itself.) In these cases, Chrome interprets the
204 response as "fully received" (in line with the specification), despite the
fact that the Python handler is still running. At this point, when the Python
handler writes the body data, it writes to any open connection it happens to
have.

In case ASCII art would help (and really, when wouldn't it?), here's a
communication flow diagram describing the behavior:


    Chrome                                  wptserve

    Request1 -----------------------------> -----------. 
    |  Request2 -------------------------->            V
    |  |                              ___  .-----------------------------.
    '<-+---------- TCP Packet #1 --- /     | HTTP/1.1 204 No Content     |
       |                             \___  |                             |
       '<--------- TCP Packet #2 --- /     | Body data (which should not |
                                     \___  | be specified)               |
                                           '-----------------------------'

So this patch is valid, and beyond beyond semantically correct, it does in fact
avoid the race condition. That said, I think it highlights a larger problem
with the tooling. @gsnedders @jgraham could either of you comment on that (and
maybe merge this patch)?

View on GitHub: https://github.com/w3c/web-platform-tests/pull/5037#issuecomment-285463942

Received on Thursday, 9 March 2017 20:00:02 UTC