JavaUrlRequest doesn't shutdown properly |
|||||||
Issue descriptionI'm seeing leaks from several tests: CronetUrlRequestTest.testUnexpectedReads CronetUrlRequestTest.testUploadReadFailAsync CronetUrlRequestTest.testUnexpectedFollowRedirects They look like this: 07-20 04:57:57.107 6669 6678 E StrictMode: A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks. 07-20 04:57:57.107 6669 6678 E StrictMode: java.lang.Throwable: Explicit termination method 'close' not called 07-20 04:57:57.107 6669 6678 E StrictMode: at dalvik.system.CloseGuard.open(CloseGuard.java:180) 07-20 04:57:57.107 6669 6678 E StrictMode: at java.net.AbstractPlainSocketImpl.create(AbstractPlainSocketImpl.java:103) 07-20 04:57:57.107 6669 6678 E StrictMode: at java.net.Socket.createImpl(Socket.java:451) 07-20 04:57:57.107 6669 6678 E StrictMode: at java.net.Socket.getImpl(Socket.java:517) 07-20 04:57:57.107 6669 6678 E StrictMode: at java.net.Socket.setSoTimeout(Socket.java:1108) 07-20 04:57:57.107 6669 6678 E StrictMode: at com.android.okhttp.Connection.connectSocket(Connection.java:195) 07-20 04:57:57.107 6669 6678 E StrictMode: at com.android.okhttp.Connection.connect(Connection.java:172) 07-20 04:57:57.107 6669 6678 E StrictMode: at com.android.okhttp.Connection.connectAndSetOwner(Connection.java:367) 07-20 04:57:57.107 6669 6678 E StrictMode: at com.android.okhttp.OkHttpClient$1.connectAndSetOwner(OkHttpClient.java:130) 07-20 04:57:57.107 6669 6678 E StrictMode: at com.android.okhttp.internal.http.HttpEngine.connect(HttpEngine.java:329) 07-20 04:57:57.107 6669 6678 E StrictMode: at com.android.okhttp.internal.http.HttpEngine.sendRequest(HttpEngine.java:246) 07-20 04:57:57.107 6669 6678 E StrictMode: at com.android.okhttp.internal.huc.HttpURLConnectionImpl.execute(HttpURLConnectionImpl.java:457) 07-20 04:57:57.107 6669 6678 E StrictMode: at com.android.okhttp.internal.huc.HttpURLConnectionImpl.connect(HttpURLConnectionImpl.java:126) 07-20 04:57:57.107 6669 6678 E StrictMode: at org.chromium.net.impl.JavaUrlRequest$7.run(JavaUrlRequest.java:696) 07-20 04:57:57.107 6669 6678 E StrictMode: at org.chromium.net.impl.JavaUrlRequest$8.run(JavaUrlRequest.java:708) 07-20 04:57:57.107 6669 6678 E StrictMode: at org.chromium.net.impl.JavaUrlRequest$SerializingExecutor$1.run(JavaUrlRequest.java:142) 07-20 04:57:57.107 6669 6678 E StrictMode: at org.chromium.net.impl.JavaUrlRequest$1$1.run(JavaUrlRequest.java:205) 07-20 04:57:57.107 6669 6678 E StrictMode: at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1133) 07-20 04:57:57.107 6669 6678 E StrictMode: at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:607) 07-20 04:57:57.107 6669 6678 E StrictMode: at org.chromium.net.impl.JavaCronetEngine$1$1.run(JavaCronetEngine.java:58) 07-20 04:57:57.107 6669 6678 E StrictMode: at java.lang.Thread.run(Thread.java:761) I tried running on a Nexus 6 flashed with Nougat but didn't see the failures. I used an x86 emulator with Nougat and could see the failures. I'm not entirely sure what's going on but I see at least one problem: JavaUrlRequest.SerializingExecutor.runTask() only runs one task and then attempts to post the next task to the underlying executor. This doesn't work well with ExecutorService.shutdown(). At shutdown time, all tasks in the queue should be executed, but because runTask() posts the next task it gets RejectedExecutionException and so only attempts to run one task. This can lead to the disconnect() task being rejected, even though it was submitted to the queue prior to initiating shutdown().
,
Jul 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78a1b1adde7d141ff0cd5c0f00b6eaa9f4ba6338 commit 78a1b1adde7d141ff0cd5c0f00b6eaa9f4ba6338 Author: Paul Jensen <pauljensen@chromium.org> Date: Tue Jul 25 15:10:14 2017 [Cronet] Fix JavaUrlRequest shutdown behavior The SerializingExecutor was previously posting tasks from its queue to the underlying Executor. This fails to execute all tasks in its queue during shutdown because the underlying Executor.execute() rejects new tasks. This causes leaks. This change makes the SerializingExecutor loop over tasks in its queue without posting to the underlying Executor. BUG= 747197 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Change-Id: I3ac1ec880f4c47e7b61a02159dce2f17f2fb7658 Reviewed-on: https://chromium-review.googlesource.com/581433 Reviewed-by: Miriam Gershenson <mgersh@chromium.org> Commit-Queue: Miriam Gershenson <mgersh@chromium.org> Cr-Commit-Position: refs/heads/master@{#489302} [modify] https://crrev.com/78a1b1adde7d141ff0cd5c0f00b6eaa9f4ba6338/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
,
Jul 25 2017
,
Jul 26 2017
Hmm I requested the merge before my fix went out in a canary build. Remove tag.
,
Jul 28 2017
,
Jul 29 2017
Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/755154e69631ebf9d94c4671209c3c8156b17ca5 commit 755154e69631ebf9d94c4671209c3c8156b17ca5 Author: Paul Jensen <pauljensen@chromium.org> Date: Mon Jul 31 13:05:56 2017 [Cronet] Fix JavaUrlRequest shutdown behavior The SerializingExecutor was previously posting tasks from its queue to the underlying Executor. This fails to execute all tasks in its queue during shutdown because the underlying Executor.execute() rejects new tasks. This causes leaks. This change makes the SerializingExecutor loop over tasks in its queue without posting to the underlying Executor. BUG= 747197 TBR=pauljensen@chromium.org (cherry picked from commit 78a1b1adde7d141ff0cd5c0f00b6eaa9f4ba6338) Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Change-Id: I3ac1ec880f4c47e7b61a02159dce2f17f2fb7658 Reviewed-on: https://chromium-review.googlesource.com/581433 Reviewed-by: Miriam Gershenson <mgersh@chromium.org> Commit-Queue: Miriam Gershenson <mgersh@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#489302} Reviewed-on: https://chromium-review.googlesource.com/593771 Reviewed-by: Paul Jensen <pauljensen@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#147} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/755154e69631ebf9d94c4671209c3c8156b17ca5/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
,
Jul 31 2017
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by pauljensen@chromium.org
, Jul 21 2017