Add connection-pool tests #24965
Add connection-pool tests #24965
Conversation
wpt-pr-bot
requested review from
domfarolino,
Hexcles,
jdm,
jgraham,
LukeZielinski,
mnot,
stephenmcgruer,
youennf and
yutakahirano
|
I've added some opaque origin tests (One of which fails in Chrome with connection partitioning enabled, due to a Chrome bug, the other of which passes). Investigated H2 tests, but Chrome doesn't look like it supports WPT H2 tests, and I'm reluctant to add any until I can verify they pass somewhere, though it may be as easy as just making a copy of the current test file that forces H2, and updating origins used in the test. No idea why sink-task is failing, or what it's actually failing on. It's unclear to me if the FireFox timeouts are because the test is stalled, or because it's slow. While I couldn't get the wpt test Python launcher to do anything but crash FireFox stable when running locally, I ran the tests on FireFox by starting the test server and navigating to it, and the tests correctly all failed due to reusing sockets, without hanging. |
We should support them upstream here in WPT (e.g. see https://wpt.fyi/results/infrastructure/server/http2-context.sub.h2.any.html?label=experimental&label=master&aligned). The downstream Chromium CI does not support them currently (see http://crbug.com/1048761); there have been recent discussions on prioritizing support but no clear movement. The more people that yell for it the quicker we'll get it done though :)
It's possible you were hit by #24924 (sorry!). If so, that issue is resolved now so if you rebase your changes to include #24992 it should no longer crash. Although what you did to test it seems reasonable :) |
|
@stephenmcgruer: Thanks, that did the trick! I can reproduce the timeouts locally (though they occur on an earlier test). Experimentally, increasing the timeout fixes the issue, locally, at least. I'd certainly love to see support for wpt tests with H2 support in the Chromium repo. I'm mostly network person, so may be a bit biased, though I haven't worked much on H2. |
|
@annevk - would you be a reasonable person to review this contribution? |
|
If folks prefer, I can get review from a Chromium person instead, just thought this was a better way to make sure folks are happy with the testing methodology, and the tests around opaque origins and Web Workers don't depend on any Chrome quirks. |
There was a problem hiding this comment.
I found a couple of nits and it would probably be good if someone else took a look as well. One thing I didn't quite understand is how connections are distinguished server side.
The other thing that's not immediately clear to me is if this can even be tested in H/1. I had the impression that each request/response pair got its own connection there.
| <html> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Connection partioning by site</title> |
| host.HTTP_NOTSAMESITE_ORIGIN | ||
| ]; | ||
|
|
||
| // This origin should correspond to a different site from the two above, but the tests |
There was a problem hiding this comment.
Maybe say "ideally corresponds" because as written it does not (and I'm not sure there's support for a third site infrastructure-wise).
| // | ||
| // include_credentials indicates whether the fetch requests use credentials or not, | ||
| // which is interesting as uncredentialed sockets have separate connection pools. | ||
| var tests = [ |
| } else if (event.data.result === "error") { | ||
| reject(event.data.details); | ||
| } else { | ||
| return; // Ignore testharness.js internal messages |
There was a problem hiding this comment.
If there would be any this would not work, right? As you also remove the listener.
There was a problem hiding this comment.
This was copied from the http cache partitioning tests - I had assumed that the test fixture added another listener and bounced around its own messages. Removing this logic doesn't make the test fail, however, so I've removed this (and instead fail the test on unexpected messages)
| } | ||
| window.removeEventListener("message", message_listener); | ||
| } | ||
| window.addEventListener("message", message_listener); |
There was a problem hiding this comment.
You can also use { once: true } as third parameter if you expect only a single call.
There was a problem hiding this comment.
You can also use
{ once: true }as third parameter if you expect only a single call.
Thanks, Anne!
That's mostly true. Connections aren't reused for responses that don't include a Content-Length response header. Responses produced by running *.py files, fortunately, all include a Content-Length header. Responses that are just the contents of some file do not. So using as few non-Python responses as possible makes the test more robust, since it prevents sockets from being closed by the server. We could look into changing behavior for non-Python responses, but decided it wasn't worth the effort, and I didn't want to change the current behavior of a huge number of tests..
I did test that these test pass in Chrome with connection partitioning, and fail without it (Except one of the opaque origin tests, as there's a Chrome bug that needs a fair bit of refactoring before it can be fixed)
|
Sorry, I meant to publish all those comments at once, but I think I posted them as individual responses? Not used to github. |
Sorry, forgot to respond to this. They're distinguished by port, which is based on the assumption that either sockets are not closed, or ports are not reused, during each test (subtest?) iteration. Generally, ports won't be reused quickly, if they were just closed, I believe. The server stores a connection to partition ID mapping for each test, and if a connection is ever used for the wrong partition, the server returns a magic error message on requests to both requests to "check_partition" and "clean_up" with that partition ID. The clean_up error shouldn't actually be necessary, just included it because those requests include test + partition IDs, so seems like they should be respected, and they happen after the check_partition requests/ |
There was a problem hiding this comment.
Thank you, looks good to me. I still recommend trying to find at least one other reviewer. I suspect @jgraham or @stephenmcgruer or some such might have to review anyway given the changes to tools/.
There was a problem hiding this comment.
LGTM for changes in tools/, with a note that I only took a brief look at the fetch python handler. (And that my networking knowledge is awful ;)).
Do I understand correctly that the expectation would be that the client host wouldn't change, but that the client port should change because the requests come from different sites and browsers are (as of whatwg/fetch#1063) required to use different client-side ports for different sites?
EDIT: Wow, I should learn to read better. Looks like Matt replied to this just above me, explaining that there is an assumption that a port won't be reused for a new connection, even though it could. This seems like a reasonable assumption, but noting we could have test flake from it. (I can't think of any other way this could be tested though!)
| # Unless nocheck_partition is true, check partition_id against server_state, and update server_state. | ||
| stash = request.server.stash | ||
| test_failed = False | ||
| if (request.GET.first(b"nocheck_partition", None) != "True"): |
There was a problem hiding this comment.
Nit; no outer parentheses needed in Python here, I believe.
|
I discovered another bug - connections are defined by (client IP, client port, server IP, server port). Client ports can be reused if the server IP or port vary. So I've added the server port to the test. I haven't added server IP - it should always be the same for a particular test, and it's not currently available. We do have hostname, but H2 can reuse connections for multi-homed hosts. I'm not sure if the H2 testing infrastructure currently supports that case, but if it does, and we add H2 tests, it seems a good idea to test it in combination with connection reuse. |
|
I'll merge this Monday unless new concerns arise. That sound okay? |
That works for me, at least. Thanks! |
This adds a couple tests for the changes landed in whatwg/fetch#1063. It opens up windows at different sites and makes sure they don't share connections. It checks fetch requests, JS resource requests, main frame requests, iframe requests, CORS requests, and dedicated worker requests. It works by providing the port a request came over to the Python server, and having it track which requests used which port.
There are a bunch of cases this doesn't currently test:
The tests also don't make sure that connections are ever shared when they should be. They also don't check connection-adjacent caches (DNS, alt service, proxy resolution, reporting, CORS cache, etc).
I've verified all tests pass in Chrome with connection partitioning on (--enable-features=PartitionConnectionsByNetworkIsolationKey), but haven't checked other browsers, nor have I exhaustively made sure that if there's a bug in a single request type the tests fail (e.g., worker source fetches are incorrectly pooled together).