Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206363002:
Added integration test to Chrome Proxy to verify direct connection on remote site timeout (Closed)
|
Created:
4 years, 4 months ago by Robert Ogden Modified:
4 years, 4 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded integration test to Chrome Proxy to verify direct connection on remote site timeout.
Added an integration test to Chrome Proxy, testing that when a remote site times out connecting, the proxy will bypass and the client will use a direct connection to the remote site. This should not take longer than 30 seconds.
This CL adds a handler to the chromeproxy-test appspot server to facilitate this test. This handler waits 60 seconds on any request coming from the proxy (causing a timeout) but responds immediately to direct connections.
BUG=635561
Committed: https://crrev.com/deb4af80f00e9455ce2cfcb61ce1fc9400462deb
Cr-Commit-Position: refs/heads/master@{#410869}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Added an integration test to Chrome Proxy to verify direct connection on proxy HTTP timeout #
Total comments: 2
Patch Set 3 : Added an integration test to Chrome Proxy to verify direct connection on proxy HTTP timeout #
Total comments: 8
Patch Set 4 : Added integration test to Chrome Proxy to verify direct connection on remote site timeout #
Total comments: 16
Patch Set 5 : Updated comments #Patch Set 6 : Updated timing thresholds for poor connections #Patch Set 7 : nit missing period #
Total comments: 4
Patch Set 8 : Updated timing to server=90s and test=120s #
Messages
Total messages: 25 (8 generated)
Description was changed from ========== Summary of change. Added an integration test to Chrome Proxy, testing that when the proxy times out or takes too long to complete an HTTP response, the client will use a DIRECT connection instead. When this happens, it should not take more than 30 seconds. BUG= ========== to ========== Summary of change. Added an integration test to Chrome Proxy, testing that when the proxy times out or takes too long to complete an HTTP response, the client will use a DIRECT connection instead. When this happens, it should not take more than 30 seconds. BUG= ==========
robertogden@chromium.org changed reviewers: + bustamante@chromium.org, sclittle@chromium.org
I'll let Richard take the first review pass here. nit: Is there a bug associated with this change? If so, could you add a crbug issue number to the BUG= line in the CL description? Also, could you change the subject in the CL description to be more descriptive than "Added an integration test to Chrome Proxy"? Just so it's easier to figure out what the test is for.
Description was changed from ========== Summary of change. Added an integration test to Chrome Proxy, testing that when the proxy times out or takes too long to complete an HTTP response, the client will use a DIRECT connection instead. When this happens, it should not take more than 30 seconds. BUG= ========== to ========== Summary of change. Added an integration test to Chrome Proxy, testing that when the proxy times out or takes too long to complete an HTTP response, the client will use a DIRECT connection instead. When this happens, it should not take more than 30 seconds. BUG=430204 ==========
Thanks for adding this! I just have some small comments below. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:334: return 'chrome_proxy_benchmark.pingback' Nit - Trailing white space should be removed. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:16: BLACKHOLE_SERVER = 'blackhole-1470152441488.appspot.com' I think you can combine these into BLACKHOLE_SERVER_URL, since the server itself isn't used anywhere. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:21: response Needs an ending period. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:31: if not self._is_chrome_proxy_enabled: This if statement isn't needed I don't think, since we'll always want to use the flags below for this test. Some cases will disable flywheel as part of the test, and the bit is used in shared methods so only valid the response headers when we think flywheel is enabled. Or in specific tests that disable it. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:32: options.AppendExtraBrowserArgs('--enable-spdy-proxy-auth') It's probably worth just calling the super ChromeProxyValidation, which will add the --enable-spdy-proxy-auth and disable quic args (there was a quic experiment that would occasionally break our tests). https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:702: self._metrics.AddResultsForPingback(tab, results) Remove trailing whitespace. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:881: via_count += 1 You probably don't need to keep track of via_count since this will throw an exception if it ever gets incremented, so the value won't be available anyways. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:889: results.AddValue(scalar.ScalarValue( It's worth adding a check to verify that bypass_count > 0, so this doesn't pass if there aren't any requests at all.
bustamante@'s comments addressed. Please re-review :) https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:334: return 'chrome_proxy_benchmark.pingback' On 2016/08/03 19:04:39, bustamante wrote: > Nit - Trailing white space should be removed. Done. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:16: BLACKHOLE_SERVER = 'blackhole-1470152441488.appspot.com' On 2016/08/03 19:04:39, bustamante wrote: > I think you can combine these into BLACKHOLE_SERVER_URL, since the server itself > isn't used anywhere. Done. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:21: response On 2016/08/03 19:04:39, bustamante wrote: > Needs an ending period. Done. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:31: if not self._is_chrome_proxy_enabled: On 2016/08/03 19:04:39, bustamante wrote: > This if statement isn't needed I don't think, since we'll always want to use the > flags below for this test. > > Some cases will disable flywheel as part of the test, and the bit is used in > shared methods so only valid the response headers when we think flywheel is > enabled. Or in specific tests that disable it. Done. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:32: options.AppendExtraBrowserArgs('--enable-spdy-proxy-auth') On 2016/08/03 19:04:39, bustamante wrote: > It's probably worth just calling the super ChromeProxyValidation, which will add > the --enable-spdy-proxy-auth and disable quic args (there was a quic experiment > that would occasionally break our tests). Done. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:702: self._metrics.AddResultsForPingback(tab, results) On 2016/08/03 19:04:39, bustamante wrote: > Remove trailing whitespace. Done. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:881: via_count += 1 On 2016/08/03 19:04:39, bustamante wrote: > You probably don't need to keep track of via_count since this will throw an > exception if it ever gets incremented, so the value won't be available anyways. Done. https://codereview.chromium.org/2206363002/diff/1/tools/chrome_proxy/integrat... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:889: results.AddValue(scalar.ScalarValue( On 2016/08/03 19:04:39, bustamante wrote: > It's worth adding a check to verify that bypass_count > 0, so this doesn't pass > if there aren't any requests at all. Done.
lgtm % one last nit Scott can you take a look? https://codereview.chromium.org/2206363002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/2206363002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:29: super(ChromeProxyClientBypassOnTimeout, self).CustomizeBrowserOptions(options) Nit - this line is over 80 characters, and should be split.
Nice code! Just a few comments, mainly about the blackholing server. Could you update the CL description to have a more detailed subject than "Summary of change"? Typically the first line of the CL description is the same as the code review subject, e.g. "Added an integration test to Chrome Proxy to verify direct connection on proxy HTTP timeout" in this case. Also, when you upload patchsets, you should name each patchset somewhat descriptively based on what changed since the previous patch set, e.g. "Addressed reviewer comments", "Added more tests for FooBar", "Moved FooBar functionality into its own file", etc. Also, it looks like the visibility of this CL is set to "Private/Protected". Unless you have a good reason, you should change it to be publicly visible, since all this code will be checked into open source anyways; just be careful not to mention confidential stuff in code review comments. Publicly visible also lets you use the commit queue to land the code after the code review is done and you've gotten LGTMs from all parties, which is almost always the right way to land any code in Chromium. https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:15: Could you explain what the blackhole server does? Does it blackhole the initial connection, or does it just blackhole after it receives the HTTP request headers from the client? https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:16: BLACKHOLE_SERVER_URL = 'http://blackhole-1470152441488.appspot.com' Where is the implementation of this? For simplicity, it might be better to implement the server support as part of the chromeproxy testserver if possible: https://cs.chromium.org/chromium/src/tools/chrome_proxy/testserver/ and the external target is http://chromeproxy-test.appspot.com/ That way, the server implementation will be present in the open source codebase here so that it can be modified easily by other people. Plus, it means that we don't have to rely on an entirely different service to be up for the tests to pass (meaning less flakiness), or worry about provisioning separately for this service. https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:19: """Tests that client bypasses proxy after proxy timeouts on HTTP nit: per Google Python style guide, the summary of the class comment or function comment should all fit on one line. https://google.github.io/styleguide/pyguide.html#Comments https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:701: self._metrics.AddResultsForPingback(tab, results) tiny nit: do you know what's up with the newline added here at the end? I don't think it's really a problem, but could you remove it if you added it accidentally? https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/proxy_timeout.py (right): https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/proxy_timeout.py:17: """ Chrome proxy test page for traffic over https. """ nit: Update this comment.
Description was changed from ========== Summary of change. Added an integration test to Chrome Proxy, testing that when the proxy times out or takes too long to complete an HTTP response, the client will use a DIRECT connection instead. When this happens, it should not take more than 30 seconds. BUG=430204 ========== to ========== Added integration test to Chrome Proxy to verify direct connection on remote site timeout. Added an integration test to Chrome Proxy, testing that when a remote site times out connecting, the proxy will bypass and the client will use a direct connection to the remote site. This should not take longer than 30 seconds. This CL adds a handler to the chromeproxy-test appspot server to facilitate this test. This handler waits 60 seconds on any request coming from the proxy (causing a timeout) but responds immediately to direct connections. BUG=635561 ==========
Test changed as discussed Wednesday. New bug=635561 Let me know what you think this time :) https://codereview.chromium.org/2206363002/diff/20001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/2206363002/diff/20001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:29: super(ChromeProxyClientBypassOnTimeout, self).CustomizeBrowserOptions(options) On 2016/08/03 21:40:55, bustamante wrote: > Nit - this line is over 80 characters, and should be split. Done. https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:19: """Tests that client bypasses proxy after proxy timeouts on HTTP On 2016/08/03 22:30:09, sclittle wrote: > nit: per Google Python style guide, the summary of the class comment or function > comment should all fit on one line. > https://google.github.io/styleguide/pyguide.html#Comments Done. https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:701: self._metrics.AddResultsForPingback(tab, results) On 2016/08/03 22:30:09, sclittle wrote: > tiny nit: do you know what's up with the newline added here at the end? I don't > think it's really a problem, but could you remove it if you added it > accidentally? bustamante@ and I both checked this out, there's not a newline at the end here when you view the file. Not sure what's up with the diff https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/proxy_timeout.py (right): https://codereview.chromium.org/2206363002/diff/40001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/proxy_timeout.py:17: """ Chrome proxy test page for traffic over https. """ On 2016/08/03 22:30:09, sclittle wrote: > nit: Update this comment. Done.
https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:15: """Check that the proxy bypasses when endpoint times out. nit: instead of "endpoint", could you say "origin"? Just so it's clear that it's not the proxy that's timing out. https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:17: If the endpoint site does not make an HTTP response in a reasonable nit: again, s/endpoint/origin/. "endpoint" is technically still correct, but replace with "origin" for consistency. https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:877: tab.WaitForDocumentReadyStateToBeComplete(timeout=30) Is 30s long enough? Doesn't the origin site (e.g. your test page) take 60s to respond? Wouldn't this depend on how long it would take for the proxy to timeout and give up on the origin? Could you also add a comment here explaining how long the timeout is expected to be? https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... File tools/chrome_proxy/testserver/app.yaml (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... tools/chrome_proxy/testserver/app.yaml:5: #application: chromeproxy-test Why did you comment these lines out? https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... File tools/chrome_proxy/testserver/server.go (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... tools/chrome_proxy/testserver/server.go:88: // delay longer than 30s test failure threshold to any request coming from the nit: looking at the rest of the file, it looks like function comments start with the function name, then a short description of what it does in complete sentences (e.g. terminated by periods or other sentence-enders). Could you make this comment match the others? https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... tools/chrome_proxy/testserver/server.go:92: // causes timeout on client, will then use direct instead nit: Could you capitalize the first word in this sentence and add a period at the end? https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... tools/chrome_proxy/testserver/server.go:93: time.Sleep(60 * time.Second); What is actually causing the bypass? Are you sure it's the client that's timing out, and not the server (e.g. appengine), or the proxy? If you are sure, could you explain in more detail what's happening here, e.g. the path of the request, and at what point the timeout/bypass happens?
Addressed sclittle@'s comments. Thanks :) https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:15: """Check that the proxy bypasses when endpoint times out. On 2016/08/09 21:31:31, sclittle wrote: > nit: instead of "endpoint", could you say "origin"? Just so it's clear that it's > not the proxy that's timing out. Done. https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:17: If the endpoint site does not make an HTTP response in a reasonable On 2016/08/09 21:31:31, sclittle wrote: > nit: again, s/endpoint/origin/. "endpoint" is technically still correct, but > replace with "origin" for consistency. Done. https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:877: tab.WaitForDocumentReadyStateToBeComplete(timeout=30) On 2016/08/09 21:31:31, sclittle wrote: > Is 30s long enough? Doesn't the origin site (e.g. your test page) take 60s to > respond? Wouldn't this depend on how long it would take for the proxy to timeout > and give up on the origin? > > Could you also add a comment here explaining how long the timeout is expected to > be? Comment added. 30 seconds comes out of a concern for user experience. The test site takes 60s so that waiting 30s will force a timeout. Either way, for the sake of the user, the proxy should bypass pretty quickly and let the user use a direct connection. https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... File tools/chrome_proxy/testserver/app.yaml (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... tools/chrome_proxy/testserver/app.yaml:5: #application: chromeproxy-test On 2016/08/09 21:31:31, sclittle wrote: > Why did you comment these lines out? New version of appengine throws up with those given. This config file was 2+ years old. I'll remove the lines for cleanliness. https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... File tools/chrome_proxy/testserver/server.go (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... tools/chrome_proxy/testserver/server.go:88: // delay longer than 30s test failure threshold to any request coming from the On 2016/08/09 21:31:32, sclittle wrote: > nit: looking at the rest of the file, it looks like function comments start with > the function name, then a short description of what it does in complete > sentences (e.g. terminated by periods or other sentence-enders). Could you make > this comment match the others? Done. https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... tools/chrome_proxy/testserver/server.go:92: // causes timeout on client, will then use direct instead On 2016/08/09 21:31:32, sclittle wrote: > nit: Could you capitalize the first word in this sentence and add a period at > the end? Done. https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/test... tools/chrome_proxy/testserver/server.go:93: time.Sleep(60 * time.Second); On 2016/08/09 21:31:32, sclittle wrote: > What is actually causing the bypass? Are you sure it's the client that's timing > out, and not the server (e.g. appengine), or the proxy? > > If you are sure, could you explain in more detail what's happening here, e.g. > the path of the request, and at what point the timeout/bypass happens? Done.
https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:877: tab.WaitForDocumentReadyStateToBeComplete(timeout=30) On 2016/08/09 21:56:22, Robert Ogden wrote: > On 2016/08/09 21:31:31, sclittle wrote: > > Is 30s long enough? Doesn't the origin site (e.g. your test page) take 60s to > > respond? Wouldn't this depend on how long it would take for the proxy to > timeout > > and give up on the origin? > > > > Could you also add a comment here explaining how long the timeout is expected > to > > be? > > Comment added. 30 seconds comes out of a concern for user experience. The test > site takes 60s so that waiting 30s will force a timeout. Either way, for the > sake of the user, the proxy should bypass pretty quickly and let the user use a > direct connection. I'm worried about the negative case, e.g. what has to break for this test to start failing, and how will it fail? 90s might be a better timeout here, so that if the 60s-delayed response goes through, this might be easier to debug. Also, >1 minute pageloads aren't uncommon on really really slow networks :). I'm a little worried about flakiness here, e.g. we don't want the test to fail just because the user's running on a slow network.
Timing updated to 90s timeout, server delays 115s. (Appspot 502's traffic at 120s) https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/2206363002/diff/60001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:877: tab.WaitForDocumentReadyStateToBeComplete(timeout=30) On 2016/08/09 22:12:03, sclittle wrote: > On 2016/08/09 21:56:22, Robert Ogden wrote: > > On 2016/08/09 21:31:31, sclittle wrote: > > > Is 30s long enough? Doesn't the origin site (e.g. your test page) take 60s > to > > > respond? Wouldn't this depend on how long it would take for the proxy to > > timeout > > > and give up on the origin? > > > > > > Could you also add a comment here explaining how long the timeout is > expected > > to > > > be? > > > > Comment added. 30 seconds comes out of a concern for user experience. The test > > site takes 60s so that waiting 30s will force a timeout. Either way, for the > > sake of the user, the proxy should bypass pretty quickly and let the user use > a > > direct connection. > > I'm worried about the negative case, e.g. what has to break for this test to > start failing, and how will it fail? 90s might be a better timeout here, so that > if the 60s-delayed response goes through, this might be easier to debug. > > Also, >1 minute pageloads aren't uncommon on really really slow networks :). I'm > a little worried about flakiness here, e.g. we don't want the test to fail just > because the user's running on a slow network. Good point. I'll make it 90 second timeout
https://codereview.chromium.org/2206363002/diff/120001/tools/chrome_proxy/tes... File tools/chrome_proxy/testserver/server.go (right): https://codereview.chromium.org/2206363002/diff/120001/tools/chrome_proxy/tes... tools/chrome_proxy/testserver/server.go:93: // Appspot will 502 traffic at 120 seconds with no response. 115 seconds seems pretty close to 120 seconds - perhaps 100 or 90 seconds would be less prone to flake? https://codereview.chromium.org/2206363002/diff/120001/tools/chrome_proxy/tes... tools/chrome_proxy/testserver/server.go:94: time.Sleep(115 * time.Second); AIUI it's probably better to make the doc.ready timeout in the test considerably longer than this delay here at the origin server, since then it's possible for the test to actually get the "You are proxy" response if the proxy doesn't timeout soon enough. That response would have the via header, and the test checks that the response doesn't have the via header, so that would cause the test to fail. That way, the delay here at the origin server is the real deadline, and the document.ready timeout in the test is just to prevent the test from running forever. Otherwise it isn't as clear if a test failure due to the 90s timeout was triggered by flakiness or a bug in the proxy.
Good thinking on the timing sclittle@, implemented as suggested https://codereview.chromium.org/2206363002/diff/120001/tools/chrome_proxy/tes... File tools/chrome_proxy/testserver/server.go (right): https://codereview.chromium.org/2206363002/diff/120001/tools/chrome_proxy/tes... tools/chrome_proxy/testserver/server.go:93: // Appspot will 502 traffic at 120 seconds with no response. On 2016/08/09 22:30:36, sclittle wrote: > 115 seconds seems pretty close to 120 seconds - perhaps 100 or 90 seconds would > be less prone to flake? Done. https://codereview.chromium.org/2206363002/diff/120001/tools/chrome_proxy/tes... tools/chrome_proxy/testserver/server.go:94: time.Sleep(115 * time.Second); On 2016/08/09 22:30:36, sclittle wrote: > AIUI it's probably better to make the doc.ready timeout in the test considerably > longer than this delay here at the origin server, since then it's possible for > the test to actually get the "You are proxy" response if the proxy doesn't > timeout soon enough. That response would have the via header, and the test > checks that the response doesn't have the via header, so that would cause the > test to fail. > > That way, the delay here at the origin server is the real deadline, and the > document.ready timeout in the test is just to prevent the test from running > forever. > > Otherwise it isn't as clear if a test failure due to the 90s timeout was > triggered by flakiness or a bug in the proxy. Ah, that's a good point. I like that logic
LGTM! Thanks for doing this.
The CQ bit was checked by robertogden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bustamante@chromium.org Link to the patchset: https://codereview.chromium.org/2206363002/#ps140001 (title: "Updated timing to server=90s and test=120s")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added integration test to Chrome Proxy to verify direct connection on remote site timeout. Added an integration test to Chrome Proxy, testing that when a remote site times out connecting, the proxy will bypass and the client will use a direct connection to the remote site. This should not take longer than 30 seconds. This CL adds a handler to the chromeproxy-test appspot server to facilitate this test. This handler waits 60 seconds on any request coming from the proxy (causing a timeout) but responds immediately to direct connections. BUG=635561 ========== to ========== Added integration test to Chrome Proxy to verify direct connection on remote site timeout. Added an integration test to Chrome Proxy, testing that when a remote site times out connecting, the proxy will bypass and the client will use a direct connection to the remote site. This should not take longer than 30 seconds. This CL adds a handler to the chromeproxy-test appspot server to facilitate this test. This handler waits 60 seconds on any request coming from the proxy (causing a timeout) but responds immediately to direct connections. BUG=635561 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Added integration test to Chrome Proxy to verify direct connection on remote site timeout. Added an integration test to Chrome Proxy, testing that when a remote site times out connecting, the proxy will bypass and the client will use a direct connection to the remote site. This should not take longer than 30 seconds. This CL adds a handler to the chromeproxy-test appspot server to facilitate this test. This handler waits 60 seconds on any request coming from the proxy (causing a timeout) but responds immediately to direct connections. BUG=635561 ========== to ========== Added integration test to Chrome Proxy to verify direct connection on remote site timeout. Added an integration test to Chrome Proxy, testing that when a remote site times out connecting, the proxy will bypass and the client will use a direct connection to the remote site. This should not take longer than 30 seconds. This CL adds a handler to the chromeproxy-test appspot server to facilitate this test. This handler waits 60 seconds on any request coming from the proxy (causing a timeout) but responds immediately to direct connections. BUG=635561 Committed: https://crrev.com/deb4af80f00e9455ce2cfcb61ce1fc9400462deb Cr-Commit-Position: refs/heads/master@{#410869} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/deb4af80f00e9455ce2cfcb61ce1fc9400462deb Cr-Commit-Position: refs/heads/master@{#410869} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
