New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 687600 link

Starred by 1 user
Status: Fixed
Owner:
█ xunji...@chromium.org
Last visit > 30 days ago
Closed: Feb 2017
Cc:
mef@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

CronetHttpURLConnection.getResponse() hangs when request body is zero-length.

Project Member Reported by xunji...@chromium.org, Feb 1 2017 
[Reported by a CronetHttpURLConnection user]

When there is a zero-length request body, Cronet's HttpURLConnection implementation spins the message loop after write() is called. The message loop is quit when UrlRequestCallback.onResponseStarted() is executed. If the user does not read from InputStream, there won't be any new tasks on the message loop. |mHasResponse| will only be set when we finish reading from InputStream. If getResponse() is called now, the message loop will block forever.

The fix is to correctly set |mHasResponse| when we received response headers, so we don't re-spin the message loop in getResponse() if we already have response headers.
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e61cddf30c3bd3490b86f87ff6ea71bf05f0368

commit 5e61cddf30c3bd3490b86f87ff6ea71bf05f0368
Author: xunjieli <xunjieli@chromium.org>
Date: Wed Feb 01 18:08:47 2017

[Cronet] Fix CronetHttpURLConnection#getResponse() hang

This CL fixes a hang case where both request body and response body are
zero-length. |mHasResponseHeadersOrCompleted| was only properly set if
client always reads response body. This is a false assumption. This CL
moves setting of |mHasResponseHeadersOrCompleted| to
onResponseStarted() callback.

R=kapishnikov@chromium.org

BUG= 687600 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2663063009
Cr-Commit-Position: refs/heads/master@{#447554}

[modify] https://crrev.com/5e61cddf30c3bd3490b86f87ff6ea71bf05f0368/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
[modify] https://crrev.com/5e61cddf30c3bd3490b86f87ff6ea71bf05f0368/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java

Comment 2 by xunji...@chromium.org, Feb 1 2017

Cc: mef@chromium.org
Labels: Merge-Request-57
Requesting merge into M57.
Project Member

Comment 3 by sheriffbot@chromium.org, Feb 1 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 1 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a744fb52679ebf065e6d910a26d58396b715dc1

commit 4a744fb52679ebf065e6d910a26d58396b715dc1
Author: xunjieli <xunjieli@chromium.org>
Date: Wed Feb 01 19:25:34 2017

[Cronet] Fix CronetHttpURLConnection#getResponse() hang

This CL fixes a hang case where both request body and response body are
zero-length. |mHasResponseHeadersOrCompleted| was only properly set if
client always reads response body. This is a false assumption. This CL
moves setting of |mHasResponseHeadersOrCompleted| to
onResponseStarted() callback.

BUG= 687600 
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2663063009
Cr-Commit-Position: refs/heads/master@{#447554}
(cherry picked from commit 5e61cddf30c3bd3490b86f87ff6ea71bf05f0368)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2666323003
Cr-Commit-Position: refs/branch-heads/2987@{#251}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/4a744fb52679ebf065e6d910a26d58396b715dc1/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
[modify] https://crrev.com/4a744fb52679ebf065e6d910a26d58396b715dc1/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java

Comment 5 by xunji...@chromium.org, Feb 1 2017

Status: Fixed (was: Started)

Comment 6 by xunji...@chromium.org, Mar 15 2017 

For future reference, the internal bug number is 34863040.

Comment 7 by pauljensen@chromium.org, Mar 15 2017 

Internal bug b/34863040
►

Sign in to add a comment