Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2164863002:
Fix CronetHttpURLConnectionTest#testServerHangsUp flake (Closed)
|
Created:
4 years, 5 months ago by xunjieli Modified:
4 years, 5 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix CronetHttpURLConnectionTest#testServerHangsUp flake
This CL makes the server to keep sending data, so when we
shut down the server, we are sure that it is still sending
(no eof has been sent).
BUG=629591
Committed: https://crrev.com/056cabaa7714222bd67ecea221b7b3d4103752ee
Committed: https://crrev.com/3627ffdcc4b7cb276a61d98e735b5d7467d06a2d
Cr-Original-Commit-Position: refs/heads/master@{#406833}
Cr-Commit-Position: refs/heads/master@{#406930}
Patch Set 1 : self review #
Total comments: 4
Patch Set 2 : address comment #
Total comments: 2
Patch Set 3 : add comment #Patch Set 4 : Fix expectation on platform stack #
Total comments: 4
Patch Set 5 : address comment #
Messages
Total messages: 35 (12 generated)
Description was changed from ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending. BUG=629591 ========== to ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 ==========
Patchset #1 (id:1) has been deleted
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
Hi Paul, thanks for finding out the flake. I was able to reproduce it on my 5X Marshmallow. I think this change should be able to fix the flake. PTAL.
https://codereview.chromium.org/2164863002/diff/20001/components/cronet/andro... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/2164863002/diff/20001/components/cronet/andro... components/cronet/android/test/native_test_server.cc:66: send.Run("HTTP/1.1 200 OK\r\nContent-Length:100000000\r\n\r\n", Is there any way we can avoid this arbitrary number? How about by simply not specifying a Content-Length?
https://codereview.chromium.org/2164863002/diff/20001/components/cronet/andro... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/2164863002/diff/20001/components/cronet/andro... components/cronet/android/test/native_test_server.cc:66: send.Run("HTTP/1.1 200 OK\r\nContent-Length:100000000\r\n\r\n", On 2016/07/20 17:40:23, pauljensen wrote: > Is there any way we can avoid this arbitrary number? How about by simply not > specifying a Content-Length? If we don't specify the Content-Length, client will just take whatever received so far (before connection closed) as the right content. There won't be any error generated.
https://codereview.chromium.org/2164863002/diff/20001/components/cronet/andro... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/2164863002/diff/20001/components/cronet/andro... components/cronet/android/test/native_test_server.cc:66: send.Run("HTTP/1.1 200 OK\r\nContent-Length:100000000\r\n\r\n", On 2016/07/20 17:46:18, xunjieli wrote: > On 2016/07/20 17:40:23, pauljensen wrote: > > Is there any way we can avoid this arbitrary number? How about by simply not > > specifying a Content-Length? > > If we don't specify the Content-Length, client will just take whatever received > so far (before connection closed) as the right content. There won't be any error > generated. 10^8 isn't really that big relative to the size of RAM or processor speed on many devices. I could imagine this getting buffered somewhere or being transfered really quickly. Could we make this larger, like 10^18? I don't think we should call this infinite if it has a definite content-length. I think when someone reads infinite they think without a content-length (e.g. a streaming response). Perhaps give it a content-length of 10^18 and call it ExabyteResponse?
Thanks! PTAL. https://codereview.chromium.org/2164863002/diff/20001/components/cronet/andro... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/2164863002/diff/20001/components/cronet/andro... components/cronet/android/test/native_test_server.cc:66: send.Run("HTTP/1.1 200 OK\r\nContent-Length:100000000\r\n\r\n", On 2016/07/20 17:55:55, pauljensen wrote: > On 2016/07/20 17:46:18, xunjieli wrote: > > On 2016/07/20 17:40:23, pauljensen wrote: > > > Is there any way we can avoid this arbitrary number? How about by simply > not > > > specifying a Content-Length? > > > > If we don't specify the Content-Length, client will just take whatever > received > > so far (before connection closed) as the right content. There won't be any > error > > generated. > > 10^8 isn't really that big relative to the size of RAM or processor speed on > many devices. I could imagine this getting buffered somewhere or being > transfered really quickly. Could we make this larger, like 10^18? > > I don't think we should call this infinite if it has a definite content-length. > I think when someone reads infinite they think without a content-length (e.g. a > streaming response). Perhaps give it a content-length of 10^18 and call it > ExabyteResponse? Done. Good point!
lgtm module comment https://codereview.chromium.org/2164863002/diff/40001/components/cronet/andro... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/2164863002/diff/40001/components/cronet/andro... components/cronet/android/test/native_test_server.cc:73: base::ThreadTaskRunnerHandle::Get()->PostTask( I think we should make this stop after sending an exabyte, or at least add a TODO to do so.
https://codereview.chromium.org/2164863002/diff/40001/components/cronet/andro... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/2164863002/diff/40001/components/cronet/andro... components/cronet/android/test/native_test_server.cc:73: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/07/21 11:30:54, pauljensen wrote: > I think we should make this stop after sending an exabyte, or at least add a > TODO to do so. Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/2164863002/#ps60001 (title: "add comment")
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 ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 ========== to ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 ========== to ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 Committed: https://crrev.com/056cabaa7714222bd67ecea221b7b3d4103752ee Cr-Commit-Position: refs/heads/master@{#406833} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/056cabaa7714222bd67ecea221b7b3d4103752ee Cr-Commit-Position: refs/heads/master@{#406833}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:60001) has been created in https://codereview.chromium.org/2172633002/ by xunjieli@chromium.org. The reason for reverting is: Broke test on the bot..
Message was sent while issue was closed.
Description was changed from ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 Committed: https://crrev.com/056cabaa7714222bd67ecea221b7b3d4103752ee Cr-Commit-Position: refs/heads/master@{#406833} ========== to ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 Committed: https://crrev.com/056cabaa7714222bd67ecea221b7b3d4103752ee Cr-Commit-Position: refs/heads/master@{#406833} ==========
Paul, mind taking another look? I talked to Matt briefly -- there isn't an easy way to make the embedded test server to send us an error after response headers that'll guarantee to be detected by the platform stack. We could spin up our own server so it can send a RST to the socket, but that's probably too much micro socket management than what is worth.
note that this has broken the Android Cronet Builder: https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20Cr...
On 2016/07/21 15:36:44, timvolodine wrote: > note that this has broken the Android Cronet Builder: > https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20Cr... also, why has this landed while android_cronet_tester trybot is still yellow?
On 2016/07/21 15:37:43, timvolodine wrote: > On 2016/07/21 15:36:44, timvolodine wrote: > > note that this has broken the Android Cronet Builder: > > > https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20Cr... > > also, why has this landed while android_cronet_tester trybot is still yellow? oh ok nevermind, this has been reverted already, pls ignore ;)
https://codereview.chromium.org/2164863002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/2164863002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:861: // On Kitkat, the default implementation doesn't throw an error and Kitkat->KitKat https://codereview.chromium.org/2164863002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:862: // reply on the client to check if the response is complete, so only reply? did you mean relies? I'm not sure they rely on the client to check, I think more likely they just assume it's complete if the TCP connection is closed. How about we just end this sentence at "error"?
https://codereview.chromium.org/2164863002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/2164863002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:861: // On Kitkat, the default implementation doesn't throw an error and On 2016/07/21 18:08:24, pauljensen wrote: > Kitkat->KitKat Done. https://codereview.chromium.org/2164863002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:862: // reply on the client to check if the response is complete, so only On 2016/07/21 18:08:24, pauljensen wrote: > reply? did you mean relies? I'm not sure they rely on the client to check, I > think more likely they just assume it's complete if the TCP connection is > closed. How about we just end this sentence at "error"? Done. Sorry i meant relies as the header has a Content-length
lgtm
The CQ bit was checked by xunjieli@chromium.org
The CQ bit was checked by xunjieli@chromium.org
On 2016/07/21 18:21:55, pauljensen wrote: > lgtm Thanks for the review!111111111111111111
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/21 18:25:35, xunjieli wrote: > On 2016/07/21 18:21:55, pauljensen wrote: > > lgtm > > Thanks for the review!111111111111111111 Hmm there are a lot of ones. Sorry, my keyboard got stuck.
Message was sent while issue was closed.
Description was changed from ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 Committed: https://crrev.com/056cabaa7714222bd67ecea221b7b3d4103752ee Cr-Commit-Position: refs/heads/master@{#406833} ========== to ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 Committed: https://crrev.com/056cabaa7714222bd67ecea221b7b3d4103752ee Cr-Commit-Position: refs/heads/master@{#406833} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 Committed: https://crrev.com/056cabaa7714222bd67ecea221b7b3d4103752ee Cr-Commit-Position: refs/heads/master@{#406833} ========== to ========== Fix CronetHttpURLConnectionTest#testServerHangsUp flake This CL makes the server to keep sending data, so when we shut down the server, we are sure that it is still sending (no eof has been sent). BUG=629591 Committed: https://crrev.com/056cabaa7714222bd67ecea221b7b3d4103752ee Committed: https://crrev.com/3627ffdcc4b7cb276a61d98e735b5d7467d06a2d Cr-Original-Commit-Position: refs/heads/master@{#406833} Cr-Commit-Position: refs/heads/master@{#406930} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3627ffdcc4b7cb276a61d98e735b5d7467d06a2d Cr-Commit-Position: refs/heads/master@{#406930} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
