Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1855653002:
net: Rewrite TestWebServer shutdown to avoid deadlock (Closed)
Descriptionnet: Rewrite TestWebServer shutdown to avoid deadlock
Android's URLConnection implementation can cause a deadlock in shutdown.
See bug for details.
Rewrite shutdown by just dropping existing requests on the server, and
join the thread. The server thread can be blocked on either the server
socket or the request socket. Need to ensure that both are closed
without races for shutdown to avoid deadlocks.
BUG=600021
Committed: https://crrev.com/0b612120ae81e866ff66061ff9fee9d2cae87645
Cr-Commit-Position: refs/heads/master@{#387501}
Patch Set 1 #Patch Set 2 : close request socket #
Total comments: 2
Patch Set 3 : ignore ioexception #Messages
Total messages: 28 (12 generated)
boliu@chromium.org changed reviewers: + phajdan.jr@chromium.org
ptal
Wait.. this CL is not yet ready. I can still reproduce a hang. Remove reviewer for now
Description was changed from ========== net: Rewrite TestWebServer shutdown to avoid deadlock Android's URLConnection implementation can cause a deadlock in shutdown. See bug for details. Rewrite shutdown by just dropping existing requests on the server, and join the thread. BUG=600021 ========== to ========== net: Rewrite TestWebServer shutdown to avoid deadlock Android's URLConnection implementation can cause a deadlock in shutdown. See bug for details. Rewrite shutdown by just dropping existing requests on the server, and join the thread. BUG=600021 ==========
boliu@chromium.org changed reviewers: - phajdan.jr@chromium.org
Description was changed from ========== net: Rewrite TestWebServer shutdown to avoid deadlock Android's URLConnection implementation can cause a deadlock in shutdown. See bug for details. Rewrite shutdown by just dropping existing requests on the server, and join the thread. BUG=600021 ========== to ========== net: Rewrite TestWebServer shutdown to avoid deadlock Android's URLConnection implementation can cause a deadlock in shutdown. See bug for details. Rewrite shutdown by just dropping existing requests on the server, and join the thread. The server thread can be blocked on either the server socket or the request socket. Need to ensure that both are closed without races for shutdown to avoid deadlocks. BUG=600021 ==========
boliu@chromium.org changed reviewers: + phajdan.jr@chromium.org
Ok, ready this time. PTAL
Could you get review from someone more familiar with Java and this code? I can then provide the OWNERS stamp if still needed.
boliu@chromium.org changed reviewers: + hush@chromium.org
On 2016/04/12 20:42:30, Paweł Hajdan Jr. wrote: > Could you get review from someone more familiar with Java and this code? > > I can then provide the OWNERS stamp if still needed. +hush for review then Looking at history of this file, majority of changes are coming from webview rather than net folks. hush fixed a similar shutdown deadlock in the cts server, where this was originally forked from years ago
Just making sure: are you running the tests infinitely over night to make sure there is no hanging? You can modify some instrumentation test code to run tests forever
On 2016/04/12 22:01:25, hush wrote: > Just making sure: are you running the tests infinitely over night to make sure > there is no hanging? You can modify some instrumentation test code to run tests > forever not overnight, but 100 iterations, which takes like 30 minutes
On 2016/04/12 22:02:04, boliu wrote: > On 2016/04/12 22:01:25, hush wrote: > > Just making sure: are you running the tests infinitely over night to make sure > > there is no hanging? You can modify some instrumentation test code to run > tests > > forever > > not overnight, but 100 iterations, which takes like 30 minutes ok, ran it overnight last night and no hang issues \o/
lgtm https://codereview.chromium.org/1855653002/diff/20001/net/test/android/javate... File net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java (right): https://codereview.chromium.org/1855653002/diff/20001/net/test/android/javate... net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java:627: if (mCurrentRequestSocket != null) { It is possible that "mCurrentRequestSocket" is already closed at this moment, which is when the current request is finished and the socket is closed at line 700. The javadoc does not say anything about whether you can call close() repeatedly. So check for mCurrentRequestSocket.isClosed() here? But if you are sure you can close a closed socket then no change needed.
LGTM
https://codereview.chromium.org/1855653002/diff/20001/net/test/android/javate... File net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java (right): https://codereview.chromium.org/1855653002/diff/20001/net/test/android/javate... net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java:627: if (mCurrentRequestSocket != null) { On 2016/04/14 17:57:36, hush wrote: > It is possible that "mCurrentRequestSocket" is already closed at this moment, > which is when the current request is finished and the socket is closed at line > 700. The javadoc does not say anything about whether you can call close() > repeatedly. So check for mCurrentRequestSocket.isClosed() here? But if you are > sure you can close a closed socket then no change needed. Hmm, that's a good question. Documentation doesn't say anything about multiple closes. And testing on the M device I have, multiple close do not trigger exception. I think in general, socket is probably tolerant of this case. I'm going to catch and ignore the IOException here though, since we want to ensure the server socket is closed as well.
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hush@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/1855653002/#ps40001 (title: "ignore ioexception")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855653002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855653002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855653002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855653002/40001
Message was sent while issue was closed.
Description was changed from ========== net: Rewrite TestWebServer shutdown to avoid deadlock Android's URLConnection implementation can cause a deadlock in shutdown. See bug for details. Rewrite shutdown by just dropping existing requests on the server, and join the thread. The server thread can be blocked on either the server socket or the request socket. Need to ensure that both are closed without races for shutdown to avoid deadlocks. BUG=600021 ========== to ========== net: Rewrite TestWebServer shutdown to avoid deadlock Android's URLConnection implementation can cause a deadlock in shutdown. See bug for details. Rewrite shutdown by just dropping existing requests on the server, and join the thread. The server thread can be blocked on either the server socket or the request socket. Need to ensure that both are closed without races for shutdown to avoid deadlocks. BUG=600021 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== net: Rewrite TestWebServer shutdown to avoid deadlock Android's URLConnection implementation can cause a deadlock in shutdown. See bug for details. Rewrite shutdown by just dropping existing requests on the server, and join the thread. The server thread can be blocked on either the server socket or the request socket. Need to ensure that both are closed without races for shutdown to avoid deadlocks. BUG=600021 ========== to ========== net: Rewrite TestWebServer shutdown to avoid deadlock Android's URLConnection implementation can cause a deadlock in shutdown. See bug for details. Rewrite shutdown by just dropping existing requests on the server, and join the thread. The server thread can be blocked on either the server socket or the request socket. Need to ensure that both are closed without races for shutdown to avoid deadlocks. BUG=600021 Committed: https://crrev.com/0b612120ae81e866ff66061ff9fee9d2cae87645 Cr-Commit-Position: refs/heads/master@{#387501} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0b612120ae81e866ff66061ff9fee9d2cae87645 Cr-Commit-Position: refs/heads/master@{#387501} |
