Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1984723002:
Support setReadTimeout in CronetHttpURLConnection (Closed)
|
Created:
4 years, 7 months ago by xunjieli Modified:
4 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, rohitagr, jdcormie, pauljensen Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport setReadTimeout in CronetHttpURLConnection
This CL adds support for setReadTimeout in
CronetHttpURLConnection. Because of late binding of sockets
to requests, we cannot support per-request connect timeout.
This CL adds an UnsupportedOperationException when
setConnectTimeout is invoked.
The implementation part of the CL is mostly taken from
a patch by rohitagr@. This CL also adds unit tests.
BUG=611851
Committed: https://crrev.com/5d1f9896cfc04ffcd76f5995c5725891a0ed8882
Cr-Commit-Position: refs/heads/master@{#394541}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : Rebased #
Total comments: 2
Patch Set 3 #Patch Set 4 : Use getConnectTimeout + getReadTimeout as the timeout for waiting for headers #
Total comments: 1
Patch Set 5 : Address John's comments #
Total comments: 8
Patch Set 6 : Address Andrei's comments #Patch Set 7 : Throw UnsupportedOperationException for setConnectTimeout #Patch Set 8 : self review #Patch Set 9 : self review #
Total comments: 1
Patch Set 10 : remove outdated include #
Total comments: 1
Messages
Total messages: 43 (17 generated)
Description was changed from ========== Support setConnectTimeout and setReadTimeout in CronetHttpURLConnection This CL is based on a patch by rohitagr@ and adds support for setConnectTimeout and setReadTimeout in CronetHttpURLConnection. BUG=611851 ========== to ========== Support setConnectTimeout and setReadTimeout in CronetHttpURLConnection This CL adds support for setConnectTimeout and setReadTimeout in CronetHttpURLConnection. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
Paul: PTAL. Thanks!
https://codereview.chromium.org/1984723002/diff/80001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1984723002/diff/80001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:519: mMessageLoop.loop(getConnectTimeout()); I don't think this is safe: as I understand it, the connect timeout is supposed to cap the amount of time it takes to establish the TCP connection, rather than the amount of time it takes to get the headers.
https://codereview.chromium.org/1984723002/diff/80001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1984723002/diff/80001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:519: mMessageLoop.loop(getConnectTimeout()); On 2016/05/16 17:21:56, rohitagr wrote: > I don't think this is safe: as I understand it, the connect timeout is supposed > to cap the amount of time it takes to establish the TCP connection, rather than > the amount of time it takes to get the headers. Yea, although the specs is pretty vague about what is a connect timeout. "Sets the maximum time in milliseconds to wait while connecting." I think this is a good proxy, although I don't really know what values users of HttpURLConnection are setting as the connect timeout. If the often-used values are very short, I agree that doing this is probably not safe.
xunjieli@chromium.org changed reviewers: + kapishnikov@chromium.org
Andrei, would you mind taking over the review? Paul is out this week. Our embedder's experiments are blocked on this. I don't want to burden you since the iOS work is P0, so let me know if I should send it to Misha.
On 2016/05/17 14:35:04, xunjieli wrote: > Andrei, would you mind taking over the review? Paul is out this week. Our > embedder's experiments are blocked on this. I don't want to burden you since the > iOS work is P0, so let me know if I should send it to Misha. Sure, I will take a look.
Hello from GMM! Thanks for taking a look at this issue! https://codereview.chromium.org/1984723002/diff/100001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1984723002/diff/100001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:408: mMessageLoop.loop(getReadTimeout()); MessageLoop.loop(int timeout) appears to pull and run tasks from the queue until it is quit() or until a single poll() blocks for more than "timeout" milliseconds. But that's pretty different than what we want here which is the following: run as many tasks from the queue as you want and block as many times as you want, but don't spend more than getReadTimeout() milliseconds in total. This version would be correct if you knew that a single task would be enough and that no other tasks can be in the queue. Even if that happens to be true today, it seems pretty brittle. Instead, consider having MessageLoop.loop() interpret "timeout" as the time budget for the whole call. Each time around the loop, compute how much time remains before that deadline and pass that number as the timeout to poll().
Description was changed from ========== Support setConnectTimeout and setReadTimeout in CronetHttpURLConnection This CL adds support for setConnectTimeout and setReadTimeout in CronetHttpURLConnection. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ========== to ========== Support setReadTimeout in CronetHttpURLConnection This CL adds support for setReadTimeout in CronetHttpURLConnection. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ==========
Thanks, John! PTAL. https://codereview.chromium.org/1984723002/diff/100001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1984723002/diff/100001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:408: mMessageLoop.loop(getReadTimeout()); On 2016/05/17 17:48:34, jdcormie wrote: > MessageLoop.loop(int timeout) appears to pull and run tasks from the queue until > it is quit() or until a single poll() blocks for more than "timeout" > milliseconds. But that's pretty different than what we want here which is the > following: run as many tasks from the queue as you want and block as many times > as you want, but don't spend more than getReadTimeout() milliseconds in total. > > This version would be correct if you knew that a single task would be enough and > that no other tasks can be in the queue. Even if that happens to be true today, > it seems pretty brittle. > > Instead, consider having MessageLoop.loop() interpret "timeout" as the time > budget for the whole call. Each time around the loop, compute how much time > remains before that deadline and pass that number as the timeout to poll(). Talked to Andrei. MessageLoop is our internal implementation detail. There is only one MessageLoop per HttpURLConnection. When user calls InputStream.read(), we will time out exactly after getReadTimeout(), so we think this what you want.
Description was changed from ========== Support setReadTimeout in CronetHttpURLConnection This CL adds support for setReadTimeout in CronetHttpURLConnection. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ========== to ========== Support setConnectTimeout and setReadTimeout in CronetHttpURLConnection This CL adds support for setConnectTimeout and setReadTimeout in CronetHttpURLConnection. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ==========
Andrei: I have changed the CL to use getConnectTimeout() + getReadTimeout() as the timeout for waiting for headers. PTAL.
https://codereview.chromium.org/1984723002/diff/80001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1984723002/diff/80001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:519: mMessageLoop.loop(getConnectTimeout()); On 2016/05/16 17:28:10, xunjieli wrote: > On 2016/05/16 17:21:56, rohitagr wrote: > > I don't think this is safe: as I understand it, the connect timeout is > supposed > > to cap the amount of time it takes to establish the TCP connection, rather > than > > the amount of time it takes to get the headers. > > Yea, although the specs is pretty vague about what is a connect timeout. "Sets > the maximum time in milliseconds to wait while connecting." > I think this is a good proxy, although I don't really know what values users of > HttpURLConnection are setting as the connect timeout. If the often-used values > are very short, I agree that doing this is probably not safe. Following up the Helen's PS4 change. I think the good approximation here would be getConnectTimeout() + getReadTimeout() value as the timeout (upper bound). That is what actually can happen here, i.e. establishing the connection and reading the headers. This is true even if connect() method is called before, since connect() method in Cronet does not guarantee that the connection was actually established. https://codereview.chromium.org/1984723002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/1984723002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:101: connection.setConnectTimeout(1000); We should also set read timeout here.
I believe you that it works today. I'm worried that it is fragile because the caller of loop(): - assumes there are no tasks already in the queue - assumes that a single task will be enough If the caller of loop() is really sure that the single next task is all he/she wants, then what's the point of the while loop in loop()? Maybe the method should be called runNextTask(int timeoutMillis)?
On 2016/05/17 20:45:00, jdcormie wrote: > I believe you that it works today. I'm worried that it is fragile because the > caller of loop(): > - assumes there are no tasks already in the queue > - assumes that a single task will be enough > > If the caller of loop() is really sure that the single next task is all he/she > wants, then what's the point of the while loop in loop()? Maybe the method > should be called runNextTask(int timeoutMillis)? That is a really good point. I will modify loop() to take into account how much time each task takes. Will update the CL when a new patch is ready.
https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:55: private Runnable take(boolean useTimeout, int timeout) throws InterruptedIOException { Should we change the timeout type in this method to long in order to avoid the long to int conversion? https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:95: long start = System.currentTimeMillis(); It is better to use System.nanoTime(), which returns a monotonically increasing value and ignores system clock adjustments. https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:108: take(false, timeout); Better take(false, 0) https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:116: if (timeElapsed < 0) { This should not be possible with System.nanoTime().
I don't really have time to review this, but I just wanted to make sure that the connect timeout somewhat follows the spec here: https://developer.android.com/reference/java/net/URLConnection.html#setConnec... Especially the part about waiting for the timeout to elapse for each IP address. This can be significant as some host names resolve to >10 IP addresses. If our net stack only supports one connection timeout per UrlRequest, one way to ensure this per-IP-address behavior is to call InetAddress.getAllByName() and multiply the user-specified timeout by the length of the IP address array, to come up with a per-UrlRequest conneciton timeout. The extra DNS resolution shouldn't slow things down as the system should just cache the result until the UrlRequest does the resolution, except in the case of the async DNS resolver, but I'm not sure we care too much about use cases involving the legacy API and the experimental async DNS resolver.
> Especially the part about waiting for the timeout to elapse for each IP address. > This can be significant as some host names resolve to >10 IP addresses. If our > net stack only supports one connection timeout per UrlRequest, one way to ensure > this per-IP-address behavior is to call InetAddress.getAllByName() and multiply > the user-specified timeout by the length of the IP address array, to come up > with a per-UrlRequest conneciton timeout. The extra DNS resolution shouldn't > slow things down as the system should just cache the result until the UrlRequest > does the resolution I don't think we want to multiply the number of IP addresses resolved with the connect timeout. If there are 10 IP addresses, and the first IP works, we don't really want to use a 10 * connectTimeout as the real connect timeout. The "warning" part sounds to me more like how their implementation does not conform to the spec and uses a larger timeout than specified. https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:55: private Runnable take(boolean useTimeout, int timeout) throws InterruptedIOException { On 2016/05/18 01:13:42, kapishnikov wrote: > Should we change the timeout type in this method to long in order to avoid the > long to int conversion? Done. https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:95: long start = System.currentTimeMillis(); On 2016/05/18 01:13:43, kapishnikov wrote: > It is better to use System.nanoTime(), which returns a monotonically increasing > value and ignores system clock adjustments. Done. https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:108: take(false, timeout); On 2016/05/18 01:13:42, kapishnikov wrote: > Better take(false, 0) Done. https://codereview.chromium.org/1984723002/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:116: if (timeElapsed < 0) { On 2016/05/18 01:13:42, kapishnikov wrote: > This should not be possible with System.nanoTime(). Done.
On 2016/05/18 11:58:53, pauljensen wrote: > I don't really have time to review this, but I just wanted to make sure that the > connect timeout somewhat follows the spec here: > https://developer.android.com/reference/java/net/URLConnection.html#setConnec... > > Especially the part about waiting for the timeout to elapse for each IP address. > This can be significant as some host names resolve to >10 IP addresses. If our > net stack only supports one connection timeout per UrlRequest, one way to ensure > this per-IP-address behavior is to call InetAddress.getAllByName() and multiply > the user-specified timeout by the length of the IP address array, to come up > with a per-UrlRequest conneciton timeout. The extra DNS resolution shouldn't > slow things down as the system should just cache the result until the UrlRequest > does the resolution, except in the case of the async DNS resolver, but I'm not > sure we care too much about use cases involving the legacy API and the > experimental async DNS resolver. I think multiplying connection timeout to the size of the IP array may not solve the multiple IP problem. Currently, the connection timeout value is not passed down to the network stack. The network stack has its own connection timeout (kTransportConnectJobTimeoutInSeconds) equal 240 seconds. Each IPv4 is tried sequentially. Here is an example, when the timeout won't work properly with hosts that are resolved to multiple IP addresses. Lets assume: 1) host is resolved to two IPs: IP1 and IP2. 2) The user sets connection timeout to 15 seconds. 3) IP1 is not responding After the multiplication, the connection timeout that will be passed to the message loop is 15sec * 2 = 30 sec. The internal implementation will try to connect to IP1 for 240 seconds until it timeouts, meanwhile the Java API will timeout in 30 seconds. As the result, IP2 will not be tried at all. If this example is correct, how can we solve this problem? Should we pass the timeout value down to the network stack. This will make the API work exactly as how it is described in the javadoc (see the link above). This will also allow the user to set timeout longer than 240 seconds.
On 2016/05/18 13:31:27, xunjieli wrote: > > Especially the part about waiting for the timeout to elapse for each IP > address. > > This can be significant as some host names resolve to >10 IP addresses. If > our > > net stack only supports one connection timeout per UrlRequest, one way to > ensure > > this per-IP-address behavior is to call InetAddress.getAllByName() and > multiply > > the user-specified timeout by the length of the IP address array, to come up > > with a per-UrlRequest conneciton timeout. The extra DNS resolution shouldn't > > slow things down as the system should just cache the result until the > UrlRequest > > does the resolution > > I don't think we want to multiply the number of IP addresses resolved with the > connect timeout. If there are 10 IP addresses, and the first IP works, we don't > really want to use a 10 * connectTimeout as the real connect timeout. > > The "warning" part sounds to me more like how their implementation does not > conform to the spec and uses a larger timeout than specified. > Actually I believe this is how the reference platform operates too, and if I'm not mistaken, how the language spec dictates.
> Actually I believe this is how the reference platform operates too, and if I'm > not mistaken, how the language spec dictates. Talked to Matt briefly. We can't support setConnectTimeout. Internally, we do late binding of sockets to requests. We only assign a socket to requests when the socket connects. This requires that sockets with the same host domain port to have same connect timeout. We can't do per-request connect timeout. I will remove the setConnectTimeout part and add an UnSupportedOperationException.
Description was changed from ========== Support setConnectTimeout and setReadTimeout in CronetHttpURLConnection This CL adds support for setConnectTimeout and setReadTimeout in CronetHttpURLConnection. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ========== to ========== Support setReadTimeout in CronetHttpURLConnection This CL adds support for setReadTimeout in CronetHttpURLConnection. Because of late binding of sockets to requests, we cannot support per-request connect timeout. This CL adds an UnsupportedOperationException when setConnectTimeout is invoked. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ==========
On 2016/05/18 15:36:38, xunjieli wrote: > > Actually I believe this is how the reference platform operates too, and if I'm > > not mistaken, how the language spec dictates. > > Talked to Matt briefly. We can't support setConnectTimeout. Internally, we do > late binding of sockets to requests. We only assign a socket to requests when > the socket connects. This requires that sockets with the same host domain port > to have same connect timeout. We can't do per-request connect timeout. > I will remove the setConnectTimeout part and add an > UnSupportedOperationException. Done.
Looks good. LGTM https://codereview.chromium.org/1984723002/diff/240001/components/cronet/andr... File components/cronet/android/test/mock_url_request_job_factory.cc (right): https://codereview.chromium.org/1984723002/diff/240001/components/cronet/andr... components/cronet/android/test/mock_url_request_job_factory.cc:12: #include "net/test/url_request/url_request_hanging_connect_job.h" This line should also be removed.
Patchset #11 (id:280001) has been deleted
xunjieli@chromium.org changed reviewers: + mef@chromium.org - pauljensen@chromium.org
Misha, could you do a OWNERs review?
lgtm
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kapishnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1984723002/#ps260001 (title: "remove outdated include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984723002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984723002/260001
Message was sent while issue was closed.
Description was changed from ========== Support setReadTimeout in CronetHttpURLConnection This CL adds support for setReadTimeout in CronetHttpURLConnection. Because of late binding of sockets to requests, we cannot support per-request connect timeout. This CL adds an UnsupportedOperationException when setConnectTimeout is invoked. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ========== to ========== Support setReadTimeout in CronetHttpURLConnection This CL adds support for setReadTimeout in CronetHttpURLConnection. Because of late binding of sockets to requests, we cannot support per-request connect timeout. This CL adds an UnsupportedOperationException when setConnectTimeout is invoked. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Support setReadTimeout in CronetHttpURLConnection This CL adds support for setReadTimeout in CronetHttpURLConnection. Because of late binding of sockets to requests, we cannot support per-request connect timeout. This CL adds an UnsupportedOperationException when setConnectTimeout is invoked. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 ========== to ========== Support setReadTimeout in CronetHttpURLConnection This CL adds support for setReadTimeout in CronetHttpURLConnection. Because of late binding of sockets to requests, we cannot support per-request connect timeout. This CL adds an UnsupportedOperationException when setConnectTimeout is invoked. The implementation part of the CL is mostly taken from a patch by rohitagr@. This CL also adds unit tests. BUG=611851 Committed: https://crrev.com/5d1f9896cfc04ffcd76f5995c5725891a0ed8882 Cr-Commit-Position: refs/heads/master@{#394541} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5d1f9896cfc04ffcd76f5995c5725891a0ed8882 Cr-Commit-Position: refs/heads/master@{#394541}
Message was sent while issue was closed.
pauljensen@chromium.org changed reviewers: + pauljensen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1984723002/diff/260001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1984723002/diff/260001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:405: throw new UnsupportedOperationException("Not supported"); I'm not sure throwing an exception here is what we want to do. I think generally for compatibility layers, where the goal is to allow old apps to use new technology, we should log an error but not throw an exception for functionality that isn't required for general use. This way embedders can try out Cronet without having to modify their code, at least for the common use case where timeouts are not hit. This is essentially what we do for things like: 1. trying to use the HTTP cache with our HttpURLConnection API impl 2. ten different JavaCronetEngine functions By throwing an exception here Cronet no longer becomes a drop in replacement they can try out. Anyhow, this is just my 2 cents. |
